materials: introduce mutable spec constants #8795
Conversation
|
Addressed comments and also tested that it works on Android. |
| @@ -208,6 +212,12 @@ class FMaterial : public Material { | |||
| } | |||
| size_t getParameters(ParameterInfo* parameters, size_t count) const noexcept; | |||
|
|
|||
| size_t getMutableConstantCount() const noexcept { return mMaterialMutableConstants.size(); } | |||
| std::optional<uint32_t> getMutableConstantId(std::string_view name) const noexcept; | |||
There was a problem hiding this comment.
uint8_t here since there's a max of 8 mutable constants?
There was a problem hiding this comment.
I changed this to a uint32_t to match the actual type of mMaterialMutableConstants.size(). I figure it's probably best keep this value as CPU word size.
| |:------|:------------------------------|:--------|-----------------| | ||
| | int | A signed, 32 bit GLSL int | | no | | ||
| | float | A single-precision GLSL float | 0.0 | no | | ||
| | bool | A GLSL bool | false | yes | |
There was a problem hiding this comment.
mention default value for mutable?
There was a problem hiding this comment.
actually, it should seem pretty obvious that it's false.
I know this was discussed in the doc, but maybe just use mutable_bool as a type would be more explicit? Because there's really not a case where you'd want mutable: false. And also mutable only applies to bool.
There was a problem hiding this comment.
I mentioned this a little bit in the design doc, but in order to keep forwards compatibility in case we want to extend this feature to other types, we should keep the spec very similar to the existing immutable constants. Treating mutable constants as different types also complicates the implementation slightly.
Rationale & design of this feature has been discussed internally. The current implementation uses a `FixedCapacityVector` to store the new program handles, but I wouldn't object to replacing it with a hasmap as discussed offline. I have compiled but not tested this yet on Android, so I'm not certain that the API bindings are correctly wired up.
64d0a41 to
1e6708e
Compare
Rationale & design of this feature has been discussed internally.
The current implementation uses a
FixedCapacityVectorto store the new program handles, but I wouldn't object to replacing it with a hasmap as discussed offline.I have compiled but not tested this yet on Android, so I'm not certain that the API bindings are correctly wired up.