engine: override spec constants on instance-basis#9774
engine: override spec constants on instance-basis#9774elizagamedev wants to merge 7 commits intomainfrom
Conversation
pixelflinger
left a comment
pixelflinger
left a comment
There was a problem hiding this comment.
This looks good, but scares the hell out of me :-)
It would be really hard to guard this change with a feature flag; but do we have high confidence that NOT using the new feature should essentially not change anything?
filament/src/PostProcessManager.cpp
Outdated
| loadMaterial(engine); | ||
| } | ||
| mMaterial->prepareProgram(driver, Variant{ variant }, CompilerPriorityQueue::CRITICAL); | ||
| mMaterial->getDefaultInstance()->prepareProgram(driver, Variant{ variant }, |
There was a problem hiding this comment.
can you have a comment that explains why it's okay to use the default instance? What if it's not the one the user ends-up using?
There was a problem hiding this comment.
You're right, I think it'd probably be best to inline the prepareProgram() call here at all the call sites of getMaterial(); will do that.
There was a problem hiding this comment.
I had to do quite a lot of refactoring to address this properly, please review.
|
|
||
| void FMaterial::compile(CompilerPriorityQueue const priority, UserVariantFilterMask variantSpec, | ||
| CallbackHandler* handler, Invocable<void(Material*)>&& callback) noexcept { | ||
| FMaterialInstance* mi = getDefaultInstance(); |
There was a problem hiding this comment.
Is Material::compile() essentially deprecated?
I feel like at least its documentation needs to change. If I understand correctly, as long as you don't set a constant on a material instance; this works like before. But if you do, this basically doesn't help you.
There was a problem hiding this comment.
Yes, that's correct. I can update the documentation.
elizagamedev
force-pushed
the
exv/mi-sc
branch
2 times, most recently
from
5c24e43 to
03d26cc
Compare
This commit adds `getConstant()` and `setConstant()` methods to `MaterialInstance`. When a material instance is created, it doesn't create or manage any spec constants on its own, but instead relies on `Material`. The client can change constants at any time, after which the necessary resources are copied and maintained by `MaterialInstance`. As part of this change, most internal rendering methods which depended on `Material` have been refactored to instead depend on `MaterialInstance`.
4 participants
This commit adds
getConstant()andsetConstant()methods toMaterialInstance.When a material instance is created, it doesn't create or manage any spec constants on its own, but instead relies on
Material. The client can change constants at any time, after which the necessary resources are copied and maintained byMaterialInstance.As part of this change, most internal rendering methods which depended on
Materialhave been refactored to instead depend onMaterialInstance.