Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support getParameter from MaterialInstance #7686

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: support getParameter from MaterialInstance #7686

wants to merge 6 commits into from

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented Mar 20, 2024 •
edited

It has been a common user request to add the ability to read values from a MaterialInstance:

This PR is an attempt to add support for this. It works for my use case, where I want to change the alpha of a material (to control an entities opacity), e.g.:

math::float4 rgba = materialInstance->getParameter<math::float4>("baseColorFactor");
float newAlpha = 0.5;
materialInstance->setParameter("baseColorFactor", math::float4({rgba.r, rgba.g, rgba.b, newAlpha}));

Changed

I basically added a getParameter function that you can pass a type to.

-> Note: I am probably missing use cases. Please let me know where I am missing something or need to watch out for. Also happy to add tests if necessary 馃槉 !

Fixes: #7683

@hannojg hannojg mentioned this pull request Mar 20, 2024
Open
@hannojg hannojg marked this pull request as ready for review March 20, 2024 15:21
@hannojg hannojg changed the title feat: support getParameter from MaterialInstance feat: support getParameter from MaterialInstance Mar 20, 2024
@hannojg hannojg changed the title feat: support getParameter from MaterialInstance feat: support getParameter from MaterialInstance Mar 20, 2024
@pixelflinger pixelflinger added the internal Issue/PR does not affect clients label Mar 20, 2024
pixelflinger
pixelflinger requested changes Apr 1, 2024
View reviewed changes
filament/src/MaterialInstance.cpp Show resolved Hide resolved
filament/include/filament/MaterialInstance.h
Comment on lines +242 to +244
* @throws utils::PreConditionPanic if name doesn't exist or no-op if exceptions are disabled.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also add one can check the paramter exists with Material::hasParameter and add a @see tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Added the @see tag , but what do you mean by "can check"?

getFieldInfo(name) already has a ASSERT_PRECONDITION that checks if the info with the given name was found, so I thought that'd be sufficient.

Where would you like me to add the hasParameter check?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant we can add in the documentation that it's possible to check the that parameter exists using hasParameter. For some applications, it's not acceptable to fail, so they can check up-front.

poweifeng
poweifeng reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@poweifeng poweifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when you try to get a texture. Or when you try to get a color, it doesn't return whether it's Linear or sRGB? If those aren't supported, maybe we need to be explicit what is supported and what is not?

filament/src/MaterialInstance.cpp
}

// explicit template instantiation of our supported types
template UTILS_PUBLIC float MaterialInstance::getParameter<float> (const char* name, size_t nameLength) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why all the spaces here?

Copy link
Contributor Author

@hannojg hannojg Apr 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, its the same here, and I thought thats how you'd prefer to have it:

filament/filament/src/MaterialInstance.cpp

Lines 109 to 111 in de3c4b6

// explicit template instantiation of our supported types
template UTILS_PUBLIC void MaterialInstance::setParameter<float> (const char* name, size_t nameLength, float const& v);
template UTILS_PUBLIC void MaterialInstance::setParameter<int32_t> (const char* name, size_t nameLength, int32_t const& v);

@hannojg
Copy link
Contributor Author

hannojg commented Apr 5, 2024 •
edited

@poweifeng you're right they are currently not supported yet.
I will look into either adding support for those, or clearly documentation what's supported and what's not.
Either on the weekend or early next week!

@pixelflinger
Copy link
Collaborator

@poweifeng you're right they are currently not supported yet. I will look into either adding support for those, or clearly documentation what's supported and what's not. Either on the weekend or early next week!

I think it's fine to document it only works for non-texture parameters for now. We can always have another PR later that adds support for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poweifeng poweifeng poweifeng left review comments

@pixelflinger pixelflinger pixelflinger approved these changes

@romainguy romainguy Awaiting requested review from romainguy

@bejado bejado Awaiting requested review from bejado

Assignees
No one assigned
Labels
internal Issue/PR does not affect clients
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Get parameter value of material from gltfio
3 participants
@hannojg @pixelflinger @poweifeng