KHR_materials_emissive_strength support #1441
Conversation
…eenz/gltf-transmission
| @@ -79,6 +79,7 @@ class gltf_render_material | |||
| double normalScale; // scale applies only to X,Y components of normal | |||
| double occlusionScale; // strength multiplier for occlusion | |||
| LLColor4 emissiveColor; // emissive mulitiplier, assumed linear encoding (spec 2.0 is silent) | |||
| double emissiveStrength; // multiplier for emissiveColor | |||
There was a problem hiding this comment.
this is confusing to me that it is double here but handled as F32 everywhere else, but seems like that is what we do for all the other fields so ok i guess
There was a problem hiding this comment.
Yeah, I was a bit weirded out by that. I'm keeping with the established convention more or less.
There was a problem hiding this comment.
Looking at the GLTF spec, it does seem there are cases where single precision floating point is enforced. From the section on "Accessors Bounds":
For floating-point components, JSON-stored minimum and maximum values represent single precision floats and SHOULD be rounded to single precision before usage to avoid any potential boundary mismatches.
Not relevant to this PR, but might be worth keeping in mind for the future. cc: @RunitaiLinden
indra/llprimitive/llgltfmaterial.h
Outdated
|
|
||
| // These fields are local to viewer and are a part of local bitmap support | ||
| typedef std::map<LLUUID, LLUUID> local_tex_map_t; | ||
| local_tex_map_t mTrackingIdToLocalTexture; |
There was a problem hiding this comment.
@AndrewMeadows changed the order of the fields to save some bits. That's relevant to resolving this merge conflict. You may be interested in choosing a location for mEmissiveStrength to optimize the packing (although the savings may only be relevant server-side)
There was a problem hiding this comment.
Yeah - I wasn't sure what the specific ordering was supposed to be here. I'll look at the git-blame and reorder as needed.
There was a problem hiding this comment.
I restored the packing from the previous merge - I'm generally assuming there's a very good reason for it.
There was a problem hiding this comment.
Grokking this further, it appears there's a comment/warning further down that LLGLTFMaterial::getHash uses mLocalTexDataDigest to determine the hash offset of the material object. (see "IMPORTANT: do not move this member down") Any fields prior to mLocalTexDataDigest will not get included in the hash. I didn't consider this before, but this probably takes precedence over @AndrewMeadows ' size optimizations.
This also would explain why the merge broke. There was a fix to GLTF hashing (and therefore render batching) in a maintenance branch.
…ure that emission strength max to something completely insane. Probably a good reason for the packing that was modified.
|
note: this PR has lots of merges in it, be sure not to do 'Squash and merge' when landing it. |
…d texture transform.
|
|
||
| void LLGLTFMaterial::setIOR(F32 ior, bool for_override) | ||
| { | ||
| mIOR = llmax(ior, 1.f); |
There was a problem hiding this comment.
According to the spec, a value of 0 is also allowed (but not (0, 1) )
| mAttenuationDistance -= FLT_EPSILON; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
According to the volume extension spec, a value of exactly zero is not allowed, and the default is +inifinity (which, coincidentally, is not a value that can be serialized to JSON, although I don't think that matters). In addition, infinity minus a finite number will evaluate to infinity, so we need something different for overrides as well.
Also we may have had a bad merge in LLGLTFMaterial.