-
Notifications
You must be signed in to change notification settings - Fork 67
IES updates #965
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
base: master
Are you sure you want to change the base?
IES updates #965
Conversation
small change to make sure `ditt` doesn't autodelete
…ubresource bitflags to not trigger asserts, update examples_tests submodule
…experimental, no real validation), update examples_tests submodule
…arguments in json payload, CI should pass now
…ate examples_tests
…ODO on how to improve it even more
… with GPU converter, encode to EF_A2R10G10B10_UNORM_PACK32, update examples_tests submodule
| inline value_t vAngle(T j) const { return (value_t)vAngles[j]; } | ||
|
|
||
| const IES_STORAGE_FORMAT sample(IES_STORAGE_FORMAT vAngle, IES_STORAGE_FORMAT hAngle) const; | ||
| template<typename T NBL_FUNC_REQUIRES(hlsl::is_same_v<T, key_t>) | ||
| inline value_t hAngle(T i) const { return (value_t)hAngles[i]; } |
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 are you doing C-style casts to the return value? its the same type already!
Also see https://github.com/Devsh-Graphics-Programming/Nabla/pull/965/files#r2690460421
| PhotometricType type; | ||
| Version version; | ||
| LuminairePlanesSymmetry symmetry; | ||
| template<typename T NBL_FUNC_REQUIRES(hlsl::is_same_v<T, key_t2>) |
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.
you're in C++, you don't need to use NBL_FUNC_REQUIRES macros, can just type > requires
| core::vector2du32_SIMD optimalIESResolution; //! optimal resolution for IES profile texture | ||
| inline key_t vAnglesCount() const { return (key_t)vAngles.size(); } | ||
| inline key_t hAnglesCount() const { return (key_t)hAngles.size(); } | ||
| inline const properties_t::base_t& getProperties() const { return static_cast<const properties_t::base_t&>(properties); } |
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 static cast is not needed !
| template<typename T NBL_FUNC_REQUIRES(hlsl::is_same_v<T, key_t2>) | ||
| inline void setValue(T ij, value_t val) { data[vAnglesCount() * ij.x + ij.y] = val; } |
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 its not in the accessor concept, then no need for it to be a templated method
| template<class ExecutionPolicy> | ||
| core::smart_refctd_ptr<asset::ICPUImageView> createIESTexture(ExecutionPolicy&& policy, const float flatten = 0.0, const bool fullDomainFlatten=false, uint32_t width = properties_t::CDC_DEFAULT_TEXTURE_WIDTH, uint32_t height = properties_t::CDC_DEFAULT_TEXTURE_HEIGHT) const; | ||
| core::smart_refctd_ptr<asset::ICPUImageView> createIESTexture(const float flatten = 0.0, const bool fullDomainFlatten=false, uint32_t width = properties_t::CDC_DEFAULT_TEXTURE_WIDTH, uint32_t height = properties_t::CDC_DEFAULT_TEXTURE_HEIGHT) 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.
You don't need flatten parameter anymore and fullDomainFlatten
| static value_t sample(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(float32_t2) uv) | ||
| { | ||
| const float32_t3 dir = octahedral_t::uvToDir(uv); |
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.
you need the texture size (can store the octahedral_t in the sampler) to convert uvs to dir (to skip the first and last half-texels)
| return 0.0f; | ||
| case symmetry_t::QUAD_SYMETRIC: //! phi MIRROR_REPEAT wrap onto [0, 90] degrees range | ||
| { | ||
| NBL_CONSTEXPR float32_t M_HALF_PI = numbers::pi<float32_t> * 0.5f; |
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.
don't use NBL_CONSTEXPR here, you need just a 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.
also rename M_HALF_PI to HalfPI
| return abs(phi); | ||
| case symmetry_t::NO_LATERAL_SYMMET: //! plot onto whole (in degress) [0, 360] range | ||
| { | ||
| NBL_CONSTEXPR float32_t M_TWICE_PI = numbers::pi<float32_t> *2.f; |
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.
| return (phi < 0.f) ? (phi + M_TWICE_PI) : phi; | ||
| } | ||
| } | ||
| return 69.f; |
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.
return NaN
| } | ||
| } |
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.
indent the cases
| // TODO: DXC seems to have a bug and cannot use symmetry_t directly with == operator https://godbolt.devsh.eu/z/P9Kc5x | ||
| const ProfileProperties::LuminairePlanesSymmetry symmetry = accessor.getProperties().getSymmetry(); |
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
s.sample(); // error: translating binary operator '==' unimplemented
works now
| const float32_t vAngle = degrees(polar.theta); | ||
| const float32_t hAngle = degrees(wrapPhi(polar.phi, symmetry)); |
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.
we have asserts in HLSL, you can assert the polar coordinates are valid (more than 0 and less than pi and 2pi respectively)
| struct impl_t | ||
| { | ||
| static uint32_t getVUB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| for (uint32_t i = 0u; i < accessor.vAnglesCount(); ++i) | ||
| if (accessor.vAngle(i) > angle) | ||
| return i; | ||
| return accessor.vAnglesCount(); | ||
| } | ||
|
|
||
| static uint32_t getHUB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| for (uint32_t i = 0u; i < accessor.hAnglesCount(); ++i) | ||
| if (accessor.hAngle(i) > angle) | ||
| return i; | ||
| return accessor.hAnglesCount(); | ||
| } | ||
| }; |
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.
you can just prefix the methods with __ as is our convention
| for (uint32_t i = 0u; i < accessor.vAnglesCount(); ++i) | ||
| if (accessor.vAngle(i) > angle) | ||
| return i; | ||
| return accessor.vAnglesCount(); | ||
| } | ||
|
|
||
| static uint32_t getHUB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| for (uint32_t i = 0u; i < accessor.hAnglesCount(); ++i) | ||
| if (accessor.hAngle(i) > angle) | ||
| return i; | ||
| return accessor.hAnglesCount(); |
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.
can you please use our upper_bound algorithm ?
| return (uint32_t)hlsl::max((int64_t)impl_t::getVUB(accessor, angle) - 1ll, 0ll); | ||
| } | ||
|
|
||
| static uint32_t getHLB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| return (uint32_t)hlsl::max((int64_t)impl_t::getHUB(accessor, angle) - 1ll, 0ll); | ||
| } | ||
|
|
||
| static uint32_t getVUB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| return (uint32_t)hlsl::min((int64_t)impl_t::getVUB(accessor, angle), (int64_t)(accessor.vAnglesCount() - 1u)); | ||
| } | ||
|
|
||
| static uint32_t getHUB(NBL_CONST_REF_ARG(accessor_t) accessor, const float32_t angle) | ||
| { | ||
| return (uint32_t)hlsl::min((int64_t)impl_t::getHUB(accessor, angle), (int64_t)(accessor.hAnglesCount() - 1u)); |
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.
please don't use uint64_t, use the return type
| logger.log("%s: Invalid IES signature for \"%s\" file!", system::ILogger::ELL_ERROR, __FUNCTION__, fName); | ||
| } | ||
| else | ||
| logger.log("%s: Failed to read \"%s\" file!", system::ILogger::ELL_ERROR, __FUNCTION__, fName); |
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're not errors, they' DEBUG or INFO, remember `isALoadableFileFormat can be called to query files if a particular loader is getting used
|
|
||
| if (height > CDC_MAX_TEXTURE_HEIGHT) | ||
| height = CDC_MAX_TEXTURE_HEIGHT; | ||
| // TODO: If no symmetry (no folding in half and abuse of mirror sampler) make dimensions odd-sized so middle texel taps the south pole |
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.
do it right now, and leave a TODO to undo it when implementing the exploitation of symmetries
| // TODO(?): should be in nbl::hlsl::ies (or in the Texutre struct) but I get | ||
| // error GA3909C62: class template specialization of 'member_count' not in a namespace enclosing 'bda' | ||
| // which I don't want to deal with rn to not (eventually) break stuff |
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.
its extremely annoying but you need to do it like this
namespace nbl
{
namespace hlsl
{
namespace ies
{
struct IESTextureInfo;
}
}
}
NBL_HLSL_DEFINE_STRUCT((::nbl::hlsl::ies::IESTextureInfo),
((inv, float32_t2))
((flatten, float32_t))
((maxValueRecip, float32_t))
((flattenTarget, float32_t))
((domainLo, float32_t))
((domainHi, float32_t))
((fullDomainFlatten, uint16_t)) // bool
);|
|
||
| struct IESTextureInfo; | ||
| NBL_HLSL_DEFINE_STRUCT((IESTextureInfo), | ||
| ((inv, float32_t2)) |
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.
give it a better name than just inv, lastTexelRcp or something
| ((flatten, float32_t)) | ||
| ((maxValueRecip, float32_t)) | ||
| ((flattenTarget, float32_t)) | ||
| ((domainLo, float32_t)) | ||
| ((domainHi, float32_t)) | ||
| ((fullDomainFlatten, uint16_t)) // bool |
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.
you don't need these members anymore because flattening is no longer needed (except for maxValueRecip)
| static inline SInfo createInfo(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(uint32_t2) size, float32_t flatten, bool fullDomainFlatten) | ||
| { | ||
| SInfo retval; | ||
| const ProfileProperties props = accessor.getProperties(); |
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.
store the info in the SProceduralTexture
| { | ||
|
|
||
| template<typename Accessor NBL_FUNC_REQUIRES(concepts::IsIESAccessor<Accessor>) | ||
| struct Texture |
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.
call it SProceduralTexture
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.
and remove the template
| static inline SInfo createInfo(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(uint32_t2) size, float32_t flatten, bool fullDomainFlatten) | ||
| { | ||
| SInfo retval; |
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.
make this into a static inline SProceduralTexture create(const float32_t minTheta, const float32_t maxTheta, const uint32_t2 resolution=Default...)
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 just static inline SProceduralTexture create(const float32_t maxCandelaValue, const uint32_t2 resolution=Default...) because flattening is no longer necessary so no need to know about the domain sizes
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.
you don't actually need to template the whole class on the accessor, just operator()
| //! max 16K resolution | ||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_MAX_TEXTURE_WIDTH = 15360u; | ||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_MAX_TEXTURE_HEIGHT = 8640u; | ||
|
|
||
| // TODO: This constraint is hack because the mitsuba loader and its material compiler use Virtual Texturing, and there's some bug with IES not sampling sub 128x128 mip levels | ||
| // don't want to spend time to fix this since we'll be using descriptor indexing for the next iteration | ||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_MIN_TEXTURE_WIDTH = 128u; | ||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_MIN_TEXTURE_HEIGHT = 128u; | ||
|
|
||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_DEFAULT_TEXTURE_WIDTH = 1024u; | ||
| NBL_CONSTEXPR_STATIC_INLINE uint32_t CDC_DEFAULT_TEXTURE_HEIGHT = 1024u; |
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.
all the texture stuff should go to SProceduralTexture
| return retval; | ||
| } | ||
|
|
||
| static inline float32_t eval(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(SInfo) info, NBL_CONST_REF_ARG(float32_t2) uv) |
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.
make this method into template operator(), its this function that needs to be templated on the accessor
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.
also hold SInfo as a member`
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 turn this into __evalNDC and use octahedral_t::ndcToDir
| const float32_t3 dir = octahedral_t::uvToDir(uv); | ||
| const polar_t polar = polar_t::createFromCartesian(dir); | ||
|
|
||
| sampler_t sampler; |
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.
sampler is a reserved keyword in HLSL, use _sampler
| const polar_t polar = polar_t::createFromCartesian(dir); | ||
|
|
||
| sampler_t sampler; | ||
| const float32_t intensity = sampler.sample(accessor, polar); |
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.
sample is a reserved keyword, turn the method into operator()
| //! blend the IES texture with "flatten" | ||
| float32_t blendV = intensity * (1.f - info.flatten); | ||
|
|
||
| const bool inDomain = (info.domainLo <= polar.theta) && (polar.theta <= info.domainHi); | ||
|
|
||
| if ((info.fullDomainFlatten && inDomain) || intensity > 0.0f) | ||
| blendV += info.flattenTarget * info.flatten; |
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.
no more flattening needed
| static inline float32_t eval(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(SInfo) info, NBL_CONST_REF_ARG(uint32_t2) position) | ||
| { | ||
| const float32_t2 uv = float32_t2(position) * info.inv; |
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.
ok so for this, you take a corner sampled unnorm position, and need to turn it into UV, so needs to have +0.5 and then mul by rcp of texsize
OR mul by rcp of texsize and add half-texel in norm coords
| using polar_t = math::Polar<float32_t>; | ||
| using octahedral_t = math::OctahedralTransform<float32_t>; | ||
|
|
||
| static value_t sample(NBL_CONST_REF_ARG(accessor_t) accessor, NBL_CONST_REF_ARG(math::Polar<float32_t>) polar) |
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.
turn sample(...) into template<typename CoordinateT> requires static value_t __call(...)
because sample is a reserved keyword and sampling is already kinda like a function call oeprator
No description provided.