-
-
Notifications
You must be signed in to change notification settings - Fork 22k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
basis_universal: Clarify encoder-only dependencies, only used in editor builds #99860
basis_universal: Clarify encoder-only dependencies, only used in editor builds #99860
Conversation
modules/basis_universal/SCsub
Outdated
if env.dev_build: | ||
env_basisu.Append(CPPDEFINES=[("BASISU_DEVEL_MESSAGES", 1), ("BASISD_ENABLE_DEBUG_FLAGS", 1)]) | ||
env_basisu.Append(CPPDEFINES=[("BASISU_FORCE_DEVEL_MESSAGES", 1), ("BASISD_ENABLE_DEBUG_FLAGS", 1)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This will change behavior in dev builds. BASISU_DEVEL_MESSAGES
doesn't exist in the current version, so what this was meant to be is BASISU_FORCE_DEVEL_MESSAGES
.
I'm not sure we actually want to see those in dev builds though, so alternatively we might remove this define to keep the current behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm erring on the side of removing both those flags actually. I don't think most engine developers benefit from a verbose basisu debugging output. Anyone working on the basisu integration can enable those temporarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards removing them outright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's give it a try, and reconsider if someone finds they're missing important dev-only info.
@@ -74,9 +81,9 @@ env_thirdparty.Append( | |||
] | |||
) | |||
|
|||
if env.editor_build: | |||
env_thirdparty.Append(CPPDEFINES=["BASISU_NO_IMG_LOADERS"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This define no longer seems to exist. I'm not sure what would be the replacement, so I just removed it, which is equivalent to how it behaves now.
a745e28
to
e067665
Compare
transcoder_sources = [thirdparty_dir + "transcoder/basisu_transcoder.cpp"] | ||
|
||
# Treat Basis headers as system headers to avoid raising warnings. Not supported on MSVC. | ||
if not env.msvc: | ||
env_basisu.Append( | ||
CPPFLAGS=["-isystem", Dir(thirdparty_dir).path, "-isystem", Dir("#thirdparty/jpeg-compressor").path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if jpeg-compressor
really needed the -isystem
trick or if we can get away with how I refactored this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes make sense to me, and if it builds I think we can be confident that it works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor shenanigans seemed to do the trick. Gonna wait for confirmation whether you wanna get rid of those flags before merging
e067665
to
fbde06e
Compare
Thanks! |
Proper solution to the issue #99825 tried to address. CC @gio3k