search

CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
937 stars 295 forks source link

UE5 build warning fixes #1321

Closed TeraDanielR closed 9 months ago

TeraDanielR commented 10 months ago

Fixes #1314 Fixes several UE5.3 build warnings when packaging.

kring commented 10 months ago

Thanks for the PR @TeraDanielR! I've pushed your changes to this branch: https://github.com/CesiumGS/cesium-unreal/tree/fix-build-warnings

That way our CI system can check for problems on other UE version and platforms. If all looks good, I'll merge this.

kring commented 9 months ago

Looks like there might be some problems on some platforms/versions. Perhaps just a missing #include. Mind taking a look? https://github.com/CesiumGS/cesium-unreal/actions/runs/7282601826

j9liu commented 9 months ago

@TeraDanielR It looks like CI is complaining about the use of #if instead of #ifdef. Do you mind changing these for the lines you added?

TeraDanielR commented 9 months ago

I will do that now! Thanks for the info!

On Mon, Jan 8, 2024 at 9:11 AM Janine Liu @.***> wrote:

@TeraDanielR https://github.com/TeraDanielR It looks like CI is complaining about the use of #if instead of #ifdef. Do you mind changing these for the lines you added?

— Reply to this email directly, view it on GitHub https://github.com/CesiumGS/cesium-unreal/pull/1321#issuecomment-1881198034, or unsubscribe https://github.com/notifications/unsubscribe-auth/BBUDZJDBKXERO762JKNE7ILYNQEB3AVCNFSM6AAAAABAVFHGTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBRGE4TQMBTGQ . You are receiving this because you were mentioned.Message ID: @.***>

kring commented 9 months ago

Sorry I don't think switching to ifdef is the right solution here (despite the error message suggesting that it is). You should be able to fix it by adding #include "CesiumCommon.h" to the source files that use these ENGINE_VERSION macros, because that's the header where they're defined. Once that's included, ENGINE_VERSION_5_3_OR_HIGHER are defined in all versions of UE, but they're defined to either true or false depending on the UE version. Using ifdef doesn't work, because #ifdef ENGINE_VERSION_5_3_OR_HIGHER will be considered true even on UE 5.2 where the value is false (because it's still defined!) when the header is included. And meanwhile #ifdef ENGINE_VERSION_5_3_OR_HIGHER will evaluate to false even on UE 5.3 when the header is not included (because it's not defined). Not sure if all that makes sense, but the short version is: switch back to #if and include the CesiumCommon.h header in any .cpp files that use these version macros.

TeraDanielR commented 9 months ago

Thanks for the info Kevin! I have pushed the changes you suggested.

On Mon, Jan 8, 2024 at 9:24 PM Kevin Ring @.***> wrote:

Sorry I don't think switching to ifdef is the right solution here (despite the error message suggesting that it is). You should be able to fix it by adding #include "CesiumCommon.h" to the source files that use these ENGINE_VERSION macros, because that's the header where they're defined. Once that's included, ENGINE_VERSION_5_3_OR_HIGHER are defined in all versions of UE, but they're defined to either true or false depending on the UE version. Using ifdef doesn't work, because #ifdef ENGINE_VERSION_5_3_OR_HIGHER will be considered true even on UE 5.2 where the value is false (because it's still defined!) when the header is included. And meanwhile #ifdef ENGINE_VERSION_5_3_OR_HIGHER will evaluate to false even on UE 5.3 when the header is not included (because it's not defined). Not sure if all that makes sense, but the short version is: switch back to #if and include the CesiumCommon.h header in any .cpp files that use these version macros.

— Reply to this email directly, view it on GitHub https://github.com/CesiumGS/cesium-unreal/pull/1321#issuecomment-1882325090, or unsubscribe https://github.com/notifications/unsubscribe-auth/BBUDZJHRBYVIOKARQFORJD3YNSZ6BAVCNFSM6AAAAABAVFHGTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBSGMZDKMBZGA . You are receiving this because you were mentioned.Message ID: @.***>

azrogers commented 9 months ago

@TeraDanielR Everything looks good here to me. Can you just push an edit to CHANGES.md adding a bullet point under Fixes for your change? Something like "Fixed deprecation warning messages when packaging in Unreal 5.3." Once that's in I can get this merged!

j9liu commented 9 months ago

I'm going to merge this and add a note to CHANGES.md afterwards. Thanks @TeraDanielR !