Forceflow / libmorton

C++ header-only library with methods to efficiently encode/decode Morton codes in/from 2D/3D coordinates
MIT License
596 stars 71 forks source link

Test suite compilation failing on Mac #49

Open kiranns opened 4 years ago

kiranns commented 4 years ago

My configuration: MacOS Mojave 10.14.6Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/c++/4.2.1 Apple LLVM version 10.0.1 (clang-1001.0.46.4) Target: x86_64-apple-darwin18.7.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Errors I am seeing: template ^ /Users/kiran/libmorton/libmorton/test/libmorton_test.h:98:107: error: expected expression return control_encode_impl<sizeof...(fields)>(std::numeric_limits::digits / sizeof...(fields), { fields... }); ^ /Users/kiran/libmorton/libmorton/test/libmorton_test.h:102:118: error: a space is required between consecutive right angle brackets (use '> >') void control_decode_impl(uint64_t encoding, std::size_t bitCount, const std::valarray<std::reference_wrapper>& fields) {

Forceflow commented 4 years ago

Hmm ... weird. I did recently drop C++11 requirement for the headers themselves, but the libmorton_test.h (part of the test suite, not the libmorton headers themselves) you're referring to still uses variadic templates, which are a C++11 feature

Does the clang version your using support that?

kiranns commented 4 years ago

I don't think it supports it. I could be mistaken, but isnt that the compilation error?

On Thu, Jun 4, 2020, 3:18 PM Jeroen Baert notifications@github.com wrote:

Hmm ... weird. I did recently drop C++11 requirement for the headers themselves, but the libmorton_test.h (part of the test suite, not the libmorton headers themselves) you're referring to still uses variadic templates, which are a C++11 feature

Does the clang version your using support that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Forceflow/libmorton/issues/49#issuecomment-639147618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETRWLWT6MBE6ENKCBTGOGDRVAMRZANCNFSM4NOGFLIQ .

kiranns commented 4 years ago

Hi Joroen,

I have another question. I would like to encode a voxel with each dimension being a uint64. Does your current library support that?

Thanks Kiran

On Thu, Jun 4, 2020, 3:18 PM Jeroen Baert notifications@github.com wrote:

Hmm ... weird. I did recently drop C++11 requirement for the headers themselves, but the libmorton_test.h (part of the test suite, not the libmorton headers themselves) you're referring to still uses variadic templates, which are a C++11 feature

Does the clang version your using support that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Forceflow/libmorton/issues/49#issuecomment-639147618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETRWLWT6MBE6ENKCBTGOGDRVAMRZANCNFSM4NOGFLIQ .

Forceflow commented 4 years ago

Well, if each of your coordinates can be 64 bits (and you sure all of those bits are used), your morton number has to be 64*3 bits = 192 bits, so you'd need some kind of 256-bit type to store that. You can probably derive methods to do that based on this library.

You sure you're not dealing with 16 / 32 bit coordinates stored in a uint64?

kiranns commented 4 years ago

I am sure. I would like to store the encoded number in an array of 3 uint64s.

On Thu, Jun 4, 2020, 4:00 PM Jeroen Baert notifications@github.com wrote:

Well, if each of your coordinates can be 64 bits (and you sure all of those bits are used), your morton number has to be 64*3 bits = 192 bits, so you'd need some kind of 256-bit type to store that.

You sure you're not dealing with 16 / 32 bit coordinates stored in a uint64?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Forceflow/libmorton/issues/49#issuecomment-639160866, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETRWLSUKA5IBQ4ETGYHMJLRVARSLANCNFSM4NOGFLIQ .

Forceflow commented 4 years ago

Well, have a look at the pre-shifted LUT method in morton3D.h

// ENCODE 3D Morton code : Pre-Shifted LookUpTable (sLUT)
    template<typename morton, typename coord>
    inline morton m3D_e_sLUT(const coord x, const coord y, const coord z) {
        morton answer = 0;
        for (unsigned int i = sizeof(coord); i > 0; --i) {
            unsigned int shift = (i - 1) * 8;
            answer =
                answer << 24 |
                (Morton3D_encode_z_256[(z >> shift) & EIGHTBITMASK] |
                    Morton3D_encode_y_256[(y >> shift) & EIGHTBITMASK] |
                    Morton3D_encode_x_256[(x >> shift) & EIGHTBITMASK]);
        }
        return answer;
    }

You can adapt this method to work with 64-bit coordinates as well, and have some logic to writeout the result to three seperate uint64's. I see it's already using sizeof(coord) in its internal logic, so that's taken care of.

And to answer your original question: if you really want to compile the test suite (which, again: is not required to use the library!), you'll have to resort to a compiler which supports C++11 / variadic templates.

Kristine1975 commented 3 years ago

Hmm ... weird. I did recently drop C++11 requirement for the headers themselves, but the libmorton_test.h (part of the test suite, not the libmorton headers themselves) you're referring to still uses variadic templates, which are a C++11 feature

Does the clang version your using support that?

It does (Apple's clang 10.0.1 equals official clang 7.0.0), but it's not turned on by default. Adding -std=c++11 to CFLAGS in the makefile fixes it:

CFLAGS=-O3 -m64 -std=c++11 -I ../libmorton/include/

However this results in further compile errors because apparently Apple defines uint_fast16_t differently: it's not unsigned int but unsigned short:

libmorton_test.cpp:121:66: note: in instantiation of function template specialization 'libmorton::m3D_e_sLUT_ET<unsigned int, unsigned short>'
      requested here
        f3D_32_encode.push_back(encode_3D_32_wrapper("LUT Shifted ET", &m3D_e_sLUT_ET<uint_fast32_t, uint_fast16_t>));
                                                                        ^
./../libmorton/include/morton3D.h:67:16: note: candidate function template not viable: no known conversion from 'const uint_fast32_t [256]' to
      'const unsigned short *' for 2nd argument
        inline morton compute3D_ET_LUT_encode(const coord c, const coord *LUT) {

Morton3D_encode_x_256 is an array of uint_fast32_t and passed to compute3D_ET_LUT_encode as its second argument. According to the explicitly specified template arguments this second argument must in this case be a pointer to uint_fast16_t, not an array/pointer to uint_fast32_t. On platforms where both uint_fast16_t and uint_fast32_t are identical (most likely unsigned int) this compiles just fine. If they are different (as on macOS), you get @kiranns compile error.

Forceflow commented 3 years ago

Does only the test suite fail to compile, or are the header files not usable too?

Kristine1975 commented 3 years ago

The following tests fail to compile. Once I commented them out, the remaining tests compiled successfully:

https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L121 https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L123 https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L158 https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L160 https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L164 https://github.com/Forceflow/libmorton/blob/7a453068e064de3a49264ae47d8afe7b51e32d6a/test/libmorton_test.cpp#L166

davidwwalker611 commented 1 year ago

I'm still getting this same problem making libmorton_test on my new MacBook using clang 14. Has the bug been fixed?

Forceflow commented 1 year ago

Can you post the full compile output?

davidwwalker611 commented 1 year ago

report.txt

Here is the compile output