Cubitect / cubiomes

C library that mimics the Minecraft biome generation.
MIT License
556 stars 98 forks source link

MSVC Compatibility Fixes #72

Closed DriftSolutions closed 1 year ago

DriftSolutions commented 1 year ago

Fixes for MSVC compatibility and added jagged_peaks and snowy_slopes to the mountains category.

Cubitect commented 1 year ago

Thanks for the report, but I see several issues with these changes.

1) The ((const)) attribute has nothing to do with inlining and it is not okay in a FORCE_INLINE definition, since it would be incorrect to use with most functions. Instead there is an ATTR(...) macro defined in rng.h which should be used to strip the attributes in a non-GNUC environment. I just failed to apply it to these functions.

2) To tell the compiler about unreachable code in a more cross compatible way, it would be better to follow something similar to the implementation of unreachable() in C23 and define an UNREACHABLE macro in rng.h.

3) The getCategory() function is a helper for the layered biome generation, where 1.18+ biomes don't exist, so I'm not sure it makes sense to add them. The concept of biome categories has essentially been replaced by climate parameters.

4) Throughout cubiomes, dynamic allocations are avoided as much as possibles for various reasons, and stack allocation should be preferred, especially for small arrays with a fixed size such as the these. y0, y1 and yn can be turned into local compile time constant by moving them to an enum. This abuses the notion of an enum a little bit, but I believe this is the most expressive solution where VLAs are not supported.

I have implemented the changes I have outlined above. Let me know if you still have problems.

DriftSolutions commented 1 year ago

That seems to fix a lot, in Visual Studio 2022 there are still 2 remaining issues from the current master:

Line 4350 needs this warning disabled, the compiler treats the negative math on unsigned as an error:

pragma warning(disable: 4146)

    b = -arg->np[i] + arg->param[idx][0];

pragma warning(default: 4146)

And in rng.h it looks like it should be:

define UNREACHABLE() __assume(0)

__assume(false) results in a compile error. This is according to https://docs.microsoft.com/en-us/cpp/intrinsics/assume?view=msvc-170

Thanks for your help/changes :)

Cubitect commented 1 year ago

Using pragmas to disable warnings is not an option since they will just cause warnings about an unknown pragma with other compilers and platforms. Also that is one of the dumbest warnings I have seen, since the integer negation needs to be on an unsigned integer, otherwise the operation is undefined when the integer overflows according to the C-standard.

Crypto2 commented 1 year ago

They can always be wrapped in #ifdef's for MSVC of course. I didn't write their compiler though I just have to live with it lol :)

Cubitect commented 1 year ago

I would generally prefer to avoid pragmas none the less. I think I've found a workaround for this case though, by simply not using a unary negation (assuming its specifically unary negations that it doesn't like).

DriftSolutions commented 1 year ago

Cool, seems to be all good now. Compiles fine and seems to be outputting the same map and everything.