Fix multiple configuration errors#1781
Conversation
RTUnreal
force-pushed
the
fix_var_compilations
branch
from
3e6999a to
f88e439
Compare
RTUnreal
force-pushed
the
fix_var_compilations
branch
from
f88e439 to
5bcdf79
Compare
ksuprynowicz
added
needs CR
JulianGro
left a comment
JulianGro
left a comment
There was a problem hiding this comment.
WebRTC is only called in the following places:
assignment-client/src/AssignmentClient.h:20:#include <shared/WebRTC.h>
domain-server/src/DomainServer.h:30:#include <shared/WebRTC.h>
libraries/audio-client/src/AudioClient.h:36:#include <shared/WebRTC.h>
libraries/networking/src/udt/NetworkSocket.h:15:#include <shared/WebRTC.h>
libraries/networking/src/webrtc/WebRTCDataChannels.h:12:#include <shared/WebRTC.h>
libraries/networking/src/webrtc/WebRTCSignalingServer.h:12:#include <shared/WebRTC.h>
libraries/networking/src/webrtc/WebRTCSocket.h:12:#include <shared/WebRTC.h>However, the Data Channel stuff is currently not actually in use, so the audio-client library should be the only place that needs to target WebRTC right now.
If you really need to target WebRTC in places that don't use it or should inherit the target, I wonder if we are doing something wrong in cmake/macros/TargetWebRTC.cmake. CMake has multiple options for defining targets, such as PRIVATE, PUBLIC, and INTERFACE. I never really properly understood those (and it always just works over here), so maybe we are missing something there.
| } | ||
| }; | ||
|
|
||
| #if defined(NVTT_API) |
There was a problem hiding this comment.
I don't think you can remove that if-statement there, since you changed it so if NVTT_API is not defined, nvtt.h won't be included. Meaning nvtt::TaskDispatcher won't be available if NVTT_API is not defined and the build will fail.
There was a problem hiding this comment.
this is-statement is encased in another NVTT_API check, which I think might be an error, because convertImageToLDRTexture and convertImageToHDRTexture are referenced in non-NVTT_API-checked code, meaning we have a hard dependency to nvtt.
There was a problem hiding this comment.
I see.
Yeah, I wouldn't be surprised if we depend on NVTT. We never tried without it, and it is actually a huge library with a LOT of functionality. We do depend on GLM though, so maybe that can take over? I don't know too much about those things.
| } | ||
| assert(numMips > 0); | ||
| Etc::RawImage *mipMaps = new Etc::RawImage[numMips]; | ||
| Etc::RawImage* mipMaps = new Etc::RawImage[numMips]; |
There was a problem hiding this comment.
Someone more knowledgable in C++ than I should review this single line.
I believe we only use ETC on Android, so I am unsure if GCC would complain about this.
There was a problem hiding this comment.
Sorry that was something I missed. My auto-formatter changed it and I forgot to revert it
RTUnreal
force-pushed
the
fix_var_compilations
branch
from
5bcdf79 to
a259847
Compare
Yes, I also didn't understand the differences between them all but I found this, which gets used in #1782, and got it now right |
|
Is there still smth, that needs to be changed here? |
JulianGro
added
CR approved
3 participants
While working on getting overte to compile with system libs, I noticed errors while getting compilation to work.
This is only to fix those issues