7

I am pretty new to C++ with Boost.

I want an object of class "world" to have an array named "chunk" of type "octreenode". Previously I had an ordinary one-dimensional array, and this worked fine. Now I'm trying to move to using a 3D array with Boost's multi_array functionality, and I'm really not sure what I'm doing wrong.

Simplified code:

class world {
public:

  typedef boost::multi_array<octreenode, 3> planetchunkarray;  // a boost_multi for chunks
  typedef planetchunkarray::index index;
  planetchunkarray *chunk;

  world(double x,double y,double z,
        int widtheast, int widthnorth, int height) :
        originx(x), originy(y), originz(z),
        chunkseast(widtheast), chunksnorth(widthnorth), chunksup(height) {

    chunk = new planetchunkarray(boost::extents[chunksnorth][chunkseast][chunksup]);
    planetchunkarray::extent_gen extents;

    for (int cz = 0; cz < chunksnorth; ++cz) {
      for (int cx = 0; cx < chunkseast; ++cx) {
        for (int cy = 0; cy < chunksup; ++cy) {
          (*chunk)[cz][cx][cy] = new octreenode(1,72);
        }
      }
    }
  }
};

After which if I attempt to make the assignment

root->planet[0]->chunk[0][0][0]->material = 4;

I get the error:

error: base operand of '->' has non-pointer type 'boost::detail::multi_array::sub_array<octreenode, 1u>'|

"octreenode" has the relevant constructor, and this line worked in identical syntax when it was just:

root->planet[0]->chunk[0]->material = 4;

(with a one-dimensional array). Similarly, while it compiled fine with a one-dimensional array, trying to pass the chunk to functions that expect a pointer to an "octreenode" object, such as:

compactoctree(root->planet[p]->chunk[cz][cx][cy], 0, 14);

generates the error

error: cannot convert 'boost::detail::multi_array::sub_array<octreenode, 1u>' to 'octreenode*' for argument '1' to 'short int compactoctree(octreenode*, int, int)'|

Would be very grateful for any suggestions, I'm sure I'm missing something obvious.

ildjarn
  • 62,044
  • 9
  • 127
  • 211
Riot
  • 15,723
  • 4
  • 60
  • 67
  • (the suggestion of dereferencing "chunk" specifically came from https://groups.google.com/forum/?fromgroups=#!topic/boost-list/IWKIdlrg4dU ) – Riot Sep 25 '12 at 00:38

1 Answers1

4

Your array is of value type (octreenode), not pointer type (octreenode*)

Therefore you are not supposed to try to assign a pointer to a dynamically allocated octreenode (new is for heap allocation, by default).

Instead, just assign a value:

      (*chunk)[cz][cx][cy] = octreenode(1,72);

In fact, there's no reason to use new on the multi array in the first place either:

UPDATE

In the comments it has been raised that more things could be optimized and that you consider that useful additions to the answer about the compilation error.

So here goes: if you indeed want to initialize all array elements with the exact same value,

  1. You can make the loops way more efficient by forgetting about the array shapes for a moment:

    std::fill_n(chunk.data(), chunk.num_elements(), octreenode {1, 72});
    

    If you know octreenode is a POD type, you could write

    std::uninitialzed_fill_n(chunk.data(), chunk.num_elements(), octreenode {1, 72});
    

    but a smart library implementation would end up calling fill_n anyways (because there's no gain). You can use uninitialized_fill_n if octreenode is not a POD type, but it is trivially destructible.

  2. In fact, there's no reason to use new on the multi array in the first place either. You can just use the constructor initialization list to construct the multi_array member


Live On Coliru

#include <boost/multi_array.hpp>
#include <type_traits>

struct octreenode { int a; int b; };

class world {
public:
    world(double x, double y, double z, int widtheast, int widthnorth, int height)
            : 
                originx(x), originy(y), originz(z), 
                chunkseast(widtheast), chunksnorth(widthnorth), chunksup(height),
                chunk(boost::extents[chunksnorth][chunkseast][chunksup])
    {
        octreenode v = { 1, 72 };
        std::fill_n(chunk.data(), chunk.num_elements(), v);
    }

private:
    double originx, originy, originz;
    int chunkseast, chunksnorth, chunksup;

    typedef boost::multi_array<octreenode, 3> planetchunkarray; // a boost_multi for chunks
    typedef planetchunkarray::index index;
    planetchunkarray chunk;
};

int main() {
    world w(1,2,3,4,5,6);
}
sehe
  • 374,641
  • 47
  • 450
  • 633
  • Since I asked this a few years ago it's a bit hard to remember the context, but the multi array is being declared on the heap because it was too large for the stack; the desire to have the chunks concurrent in memory was the intention behind allocating the multi-array of non-pointers on the heap, rather than an array of pointers which could be all over the place, and would thrash the cache while dereferencing and iterating. However, I can see looking back now that my attempt to "new octreenode" in that way was misplaced, and i should have used the placement form of new. I'll post an answer. – Riot Jan 12 '15 at 17:35
  • 2
    i hope not! Your answer prompted me to revisit this, and hopefully this discussion will be of use to someone coming here from google in the future. – Riot Jan 12 '15 at 18:31
  • @Riot Cough. It _does_ teach me that. There was **nothing** in the original post that makes your own answer more applicable. Also, I've just explained at your answer why it's bad for your health. So, tell me again how I should feel happy about the fact that I (considerable) spent time on an old answer, with the upshot that "someone coming here from google in the future" can get mediocre answers because two conflicting answers exist and the correct one didn't even have a single vote? – sehe Jan 13 '15 at 10:47
  • If having the inapplicability of your answer to the original problem pointed out to you causes this level of anguish, then perhaps you are right not to want to spend so much time on it; certainly the comments you've made on my accepted (and downvoted by you presumably) answer might have been useful contributions in an answer of their own; but that's not the answer you actually posted, is it? You can claim that using placement new is "bad for your health" but it's a useful and standard feature of the language and this is one example where its use is perfectly valid. I'm sorry you're so upset. – Riot Jan 15 '15 at 05:28
  • 3
    "and this is one example where its use is perfectly valid". No it isn't. That's the whole point. **A.** It's only valid if the `octreenode` is trivially destructible **B.** It's useless complicated even then, since it [codegens to /exactly/ the same object code](http://stackoverflow.com/questions/12574682/pointers-to-a-class-in-dynamically-allocated-boost-multi-array-not-compiling/27868498#comment44237773_27907784). The assignment in my answer is more simpler, more correct, and exactly as efficient. So, answer is **more** applicable :) – sehe Jan 15 '15 at 07:26
  • Not even remotely. Your original answer assigned pointers in an array to objects that could be anywhere on the heap. Mine assigns them in a contiguous group. You can't really give a totally different answer, then go raging in the comments that the answer you **meant** but didn't **write** is more efficient, and only after I replied with regards to your **original** answer! The new points in your "update" are perfectly valid but it's ridiculous to get angry at me for responding to what you originally wrote before you appended those later, and then act as if those were there all along. – Riot Jan 15 '15 at 09:00
  • 1
    @Riot No my original answer [did **nothing** of the sort](http://stackoverflow.com/revisions/27868498/1) (it should be immediately clear for any C++ programmer since I did never use any heap allocation). You just don't know how misguided your reasoning is here. The **one** class that governs the locality of the backing storage is `multi_array` in this respect. It wouldn't be an array if the storage wasn't contiguous. – sehe Jan 15 '15 at 09:04
  • Also. I updated my answer upon your _specific_ suggestion (should I say _reprimand_? [_"but that's not the answer you actually posted, is it?"_](http://tinyurl.com/jw8lrxr) and [_"If you post that as an answer i'm sure it would also be valid."_](http://tinyurl.com/mpql7hg)). I already explained why I didn't initially post those. I'm not going to revert it now. You have the edit history to check facts about your claims. – sehe Jan 15 '15 at 09:10
  • No of course; but in the original example I used heap allocation, and should have made clear that this was intentional because the size of the data was in the order of gigabytes. I took your original answer to mean you were allocating each octreenode on the heap and just keeping the multi_array of pointers on the stack; now I read back I realise I misunderstood your intention, and it's not your fault the answer was unsuitable because I failed to spell out in my question the reason for the heap requirement. I'm quite tired of arguing about code i wrote years ago and would not write now, though. – Riot Jan 15 '15 at 09:10
  • 1
    @Riot Okay. Fair enough. I can see this misinterpretation. I prefer to discuss actual programs (which is also why I care to provide *live demos*). The heap allocation argument doesn't make much sense to me because `multi_array` by definition uses heap allocation (or whatever your allocator uses). And you can even `heap_world = new world(...)` to have _literally_ everything related on the heap. (Introducing indirections invariably makes the representation _less_ efficient. But I know it can be beneficial depending on use cases). Cheers! – sehe Jan 15 '15 at 09:17
  • Yes, that's exactly what I meant when I said that this is code I would not write now in the first place :) I'm glad we're on the same page about it – Riot Jan 15 '15 at 22:54