-
Notifications
You must be signed in to change notification settings - Fork 416
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
[PBR][IBL] Upgrade the cubemap #1412
Conversation
src/esp/scene/SceneGraph.h
Outdated
// TODO: remove this | ||
gfx::DrawableGroup& getDrawables() { | ||
return drawableGroups_.at(std::string{}); | ||
gfx::DrawableGroup& getDrawables(const char* groupName = "") { |
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.
Why char pointers as args instead of std::string?
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 am passing a string literal to this function, so I use const char*
here. It would be faster if std::string
can be avoided.
But here it will be converted to the std::string
anyway, so I guess you are right, I can just use string directly.
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.
@bigbike Couldn't you use corrade::stringview or something to avoid a copy?
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.
corrade::stringview
@Skylion007 : No, that is exactly for the C string literals. To avoid the copy then the data structure should not use std::string
as the key which I doubt.
src/esp/scene/SceneGraph.h
Outdated
} | ||
const gfx::DrawableGroup& getDrawables(const char* groupName = "") const { | ||
return drawableGroups_.at(std::string{groupName}); | ||
const gfx::DrawableGroup& getDrawables(std::string groupName = "") const { |
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.
Actually, shoudln't these be const refs?
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 do not think so since reference is rvalue.
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.
Wait, it is const string&
so it would work (compile).
But it will not help since it will construct a string anyway (because the input parameter is a string literal.)
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.
Sorry If I'm going too overboard with those microoptimizations, spending a lot of time in low-level code lately and it shows 😅
src/esp/gfx/CubeMap.cpp
Outdated
void textureTypeSanityCheck(CubeMap::Flags& flag, | ||
CubeMap::TextureType type, | ||
const std::string& functionNameStr) { | ||
void textureTypeSanityCheck(const std::string& functionNameStr, |
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 might be overdoing the optimizations, but since the string is used only for assertions and thus should be as zero-overhead as possible, maybe go with just
void textureTypeSanityCheck(const std::string& functionNameStr, | |
void textureTypeSanityCheck(const char* functionNameStr, |
instead?
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.
Ha, thanks! Good suggestions.
src/esp/gfx/CubeMap.cpp
Outdated
case CubeMap::TextureType::Count: | ||
break; | ||
} | ||
CORRADE_INTERNAL_ASSERT_UNREACHABLE(); | ||
} | ||
|
||
/** @brief do a couple of sanity checks based on mipLevel value */ | ||
void mipLevelSanityCheck(const std::string& msgPrefix, |
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.
Same as above, maybe this instead?
void mipLevelSanityCheck(const std::string& msgPrefix, | |
void mipLevelSanityCheck(const char* msgPrefix, |
src/esp/gfx/CubeMap.cpp
Outdated
Mn::Vector2i viewportSize{int(framebufferSize), | ||
int(framebufferSize)}; // at mip level 0 |
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.
Code golf -- the single-argument constructor is there exactly for this:
Mn::Vector2i viewportSize{int(framebufferSize), | |
int(framebufferSize)}; // at mip level 0 | |
Mn::Vector2i viewportSize{int(framebufferSize)}; // at mip level 0 |
About the casts, I ended up using Vector2i
instead of Vector2ui
for sizes because the extra annoyance with unsigned types wasn't worth it. So maybe here just int framebufferSize
could be fine as well.
src/esp/gfx/CubeMap.cpp
Outdated
mipmapLevels_); | ||
|
||
if ((flags_ & Flag::ManuallyBuildMipmap) && (flags_ & Flag::ColorTexture)) { | ||
int size = imageSize_ / pow(2, mipLevel); |
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.
int size = imageSize_ / pow(2, mipLevel); | |
int size = imageSize_ / (1 << mipLevel); |
(I used to have Math::pow2(n)
for this but ... it felt weird to have that wrapping just a bit shift, so it's no longer there.)
src/esp/gfx/CubeMap.cpp
Outdated
0, // color attachment | ||
Mn::Color4{0, 0, 0, 1} // clear color | ||
0, // color attachment | ||
Mn::Color4{0.0, 0.0, 0.0, 1.0} // clear color |
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.
If you go the extra way, why not extra extra :D
Mn::Color4{0.0, 0.0, 0.0, 1.0} // clear color | |
Mn::Color4{0.0f, 0.0f, 0.0f, 1.0f} // clear color |
(I wonder, did the compiler warn here? My assumption is that it shouldn't have for the integer literals, but it should for the double literals?)
src/esp/gfx/CubeMap.cpp
Outdated
int size = imageSize_ / pow(2, mipLevel); | ||
|
||
#ifndef MAGNUM_TARGET_WEBGL | ||
CORRADE_ASSERT(texture.imageSize(0) == Mn::Vector2i(size, size), |
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.
FYI, the single-argument constructor does exactly this:
CORRADE_ASSERT(texture.imageSize(0) == Mn::Vector2i(size, size), | |
CORRADE_ASSERT(texture.imageSize(0) == Mn::Vector2i{size}, |
src/esp/scene/SceneGraph.h
Outdated
// TODO: remove this | ||
gfx::DrawableGroup& getDrawables() { | ||
return drawableGroups_.at(std::string{}); | ||
gfx::DrawableGroup& getDrawables(const std::string& groupName = "") { |
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.
gfx::DrawableGroup& getDrawables(const std::string& groupName = "") { | |
gfx::DrawableGroup& getDrawables(const std::string& groupName = {}) { |
std::string
initialization from an empty C string literal involves a lot of nasty code including a strlen()
, calling the default constructor not.
Same below.
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, good to know.
I was thinking the empty string was OK, since strlen()
returned 0.
Motivation and Context
Refactor the cubemap so that it can be used in the PBR IBL.
How Has This Been Tested
Types of changes
Checklist