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.

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

Feature Implemeted -- Multiple Material support for geometry #5774

Merged
merged 7 commits into from
Sep 18, 2022

Conversation

ShenpaiSharma
Copy link
Contributor

Resolves #3806 #5308

Changes:

Implemented a few new uniforms (uSpecularMatColor, uAmbientMatColor, uEmissiveMatColor) which store the RGB colors for different materials and fill the geometry with different color properties and thus contributing separately to the lighting of the surface.

So finally, we have decided to move away from the current overwriting of fill colors to the geometry, towards the approach that Processing uses where the material has different color properties (ambient, emissive, specular) that all contribute separately to the lighting of the surface.

Screenshots of the change:

Before:
image

Code

After:
image

Code

PR Checklist

@Qianqianye
Copy link
Contributor

Thanks @ShenpaiSharma. I am inviting the GSoC mentor @calebfoss to review this PR.

@calebfoss
Copy link
Contributor

calebfoss commented Sep 5, 2022

Awesome work, @ShenpaiSharma!

I'm thinking it would be helpful to update the inline documentation a bit to clarify that specular, emissive, ambient, and fill can be combined, but normal cannot.

Perhaps in normalMaterial, we can add a sentence to the effect of "This material cannot be combined with other materials and will override the current material settings." In ambientMaterial, I think it would clarify to revise "Sets the current material as an ambient material of the given color. The ambientMaterial() color is the color the object will reflect under any lighting." to something like "Sets the ambient color of the current material. Objects will reflect this color under any lighting. The ambient color is combined with the current fill color as well as colors set with emissiveMaterial() and specularMaterial()." And similar adjustments to emissiveMaterial and specularMaterial.

Also something I'd be interested to hear more thoughts on before merging this PR:

  • Combining material qualities/colors raises the question of how to turn these qualities off. What if, for example, you want to give one shape a specular color but then you want the following shape to not use specular? In this implementation, the way you would do that is to call specularMaterial(0). I'm wondering if that is sufficiently intuitive or if we might want to add methods called noSpecularMaterial, noEmissiveMaterial, and noAmbientMaterial to echo the 2D api?
  • Now that these methods set qualities of the current material, rather than setting the renderer to use a new material, would it be more intuitive to drop the "material" in their names (i.e. specular(), emissive(), and ambient() like in Processing)? This could also help clarify the difference between these methods and normalMaterial().
    @kjhollen @stalgiag @aferriss - If you have a chance, it would be helpful to hear your perspective!

@kjhollen
Copy link
Member

kjhollen commented Sep 6, 2022

Congrats @ShenpaiSharma!

  • Now that these methods set qualities of the current material, rather than setting the renderer to use a new material, would it be more intuitive to drop the "material" in their names (i.e. specular(), emissive(), and ambient() like in Processing)? This could also help clarify the difference between these methods and normalMaterial().
    @kjhollen @stalgiag @aferriss - If you have a chance, it would be helpful to hear your perspective!

I think that early on we discussed adding the word "material" to match more present-day tools that use shaders (Unity, three.js, etc) so that people familiar with other platforms would have a consistent search term. This vocabulary was less common in the early days of Processing, so I think it's ok not to match here! Curious what others think.

@aferriss
Copy link
Contributor

aferriss commented Sep 6, 2022

Combining material qualities/colors raises the question of how to turn these qualities off. What if, for example, you want to give one shape a specular color but then you want the following shape to not use specular? In this implementation, the way you would do that is to call specularMaterial(0). I'm wondering if that is sufficiently intuitive or if we might want to add methods called noSpecularMaterial, noEmissiveMaterial, and noAmbientMaterial to echo the 2D api?

I might be wrong, but aren't these function calls affected by calls to push() and pop() ? Do you think that's not enough of a handle to help people control turning on and off material properties?

@calebfoss
Copy link
Contributor

@kjhollen, thanks for that insight! Sounds like leaving the method names as is would be more clear, particularly with the adjustments to documentation.

I might be wrong, but aren't these function calls affected by calls to push() and pop() ? Do you think that's not enough of a handle to help people control turning on and off material properties?

@aferriss, that's correct! My thinking is that stroke() and fill() are also affected by push() and pop(), but we still have noStroke() and noFill(). That being said, push() and pop() may be more familiar to folks using webGL mode, since they'll need to use transformations to position models anyway.

Here's an example of drawing two spheres without using push() and pop(), one with specularMaterial and one without:
https://editor.p5js.org/cfoss/sketches/9Ocp292Ks

@aferriss
Copy link
Contributor

aferriss commented Sep 6, 2022

@calebfoss Thanks for the context and the example! I definitely see the value in what you're proposing and it makes sense to me.

Might be worth starting another thread with this proposal just to keep things clean here, since I don't think your proposed feature would be a blocking issue for this PR.

@stalgiag
Copy link
Contributor

stalgiag commented Sep 7, 2022

Congrats @ShenpaiSharma!

  • Now that these methods set qualities of the current material, rather than setting the renderer to use a new material, would it be more intuitive to drop the "material" in their names (i.e. specular(), emissive(), and ambient() like in Processing)? This could also help clarify the difference between these methods and normalMaterial().
    @kjhollen @stalgiag @aferriss - If you have a chance, it would be helpful to hear your perspective!

I think that early on we discussed adding the word "material" to match more present-day tools that use shaders (Unity, three.js, etc) so that people familiar with other platforms would have a consistent search term. This vocabulary was less common in the early days of Processing, so I think it's ok not to match here! Curious what others think.

For the previous behavior, it felt nice to break from Processing and use "material" for the reasons that @kjhollen listed here. Though now that these methods don't switch the set of shaders used for rendering, the word 'material' at the end feels less accurate because they aren't changing what I would think of as the material but instead setting attributes of a single material. It almost feels like if we keep the term 'material' then the sequence should be reversed to communicate that the material is being modified not swapped (ie: materialSpecular(), materialEmissive() ). This feels clunky though because the unique segment of the name is at the end and the differentiation from instances where the material is actually being swapped becomes more confusing. This is a conundrum and the Processing names begin to sound better as I consider it. But I am open to anything and will follow the group's lead.

btw great work @ShenpaiSharma !!

@calebfoss
Copy link
Contributor

Thanks, @aferriss! Happy to get that started once this is merged.

For the previous behavior, it felt nice to break from Processing and use "material" for the reasons that @kjhollen listed here. Though now that these methods don't switch the set of shaders used for rendering, the word 'material' at the end feels less accurate because they aren't changing what I would think of as the material but instead setting attributes of a single material. It almost feels like if we keep the term 'material' then the sequence should be reversed to communicate that the material is being modified not swapped (ie: materialSpecular(), materialEmissive() ). This feels clunky though because the unique segment of the name is at the end and the differentiation from instances where the material is actually being swapped becomes more confusing. This is a conundrum and the Processing names begin to sound better as I consider it. But I am open to anything and will follow the group's lead.

@stalgiag, thanks for these thoughts! Yes, I think if I were seeing specularMaterial() for the first time, I would assume that the renderer would now use a new, shiny material, which is exactly what it's been doing until this feature. So I was a bit concerned if we change these methods' behaviors without changing their names, that might cause some confusion.

So here's what these variations look like when producing this example:
image

A

ambientMaterial(128, 0, 0);
emissiveMaterial(0, 0, 128);
specularMaterial(0, 128, 0);
sphere(width/4);

B

materialAmbient(128, 0, 0);
materialEmissive(0, 0, 128);
materialSpecular(0, 128, 0);
sphere(width/4);

C

ambient(128, 0, 0);
emissive(0, 0, 128);
specular(0, 128, 0);
sphere(width/4);

While I certainly see the value in being consistent with other tools (as @kjhollen referenced) and in being consistent with how things have been named already, looking at these options, I think C reads the most clear.

A bonus benefit if we do decide to add new methods to turn these off is that noSpecular() is simpler to type and read than noSpecularMaterial() and I think clearer as well.

@ShenpaiSharma ShenpaiSharma force-pushed the Multiple_Mat branch 2 times, most recently from 89506b2 to b1baa59 Compare September 13, 2022 16:56
@kjhollen
Copy link
Member

Being mindful of the GSoC project timeframe, I don't think it's essential to change the method names immediately as a condition of this merge.

We could decide to open a new issue and track renaming the functions as future work, as I think there's a little more to do beyond just changing the method names. For example, can we deprecate the methods as they exist now, so that we don't break anyone's code on short notice? Would we want some custom friendly error messages that indicate that the method name was/is changing?

In terms of methods for turning off types of materials, we do also have resetShader() and should think about how that relates to built-in materials.

Is there any additional discussion needed before merging this PR? I know @aceslowman is eager to use these new features in a web GL tutorial. Please let me know if I can help with any further review!

@calebfoss calebfoss merged commit fb6ce7e into processing:main Sep 18, 2022
@calebfoss
Copy link
Contributor

@kjhollen this makes sense, and thank you for the additional info. This is now merged, and I'm happy to open up renaming conversations in a new thread. Sorry for holding things up!

Congratulations and thank you, @ShenpaiSharma !!!

@ShenpaiSharma ShenpaiSharma deleted the Multiple_Mat branch September 19, 2022 07:12
@kjhollen
Copy link
Member

thanks @calebfoss and congrats @ShenpaiSharma!! 🥳

@stalgiag
Copy link
Contributor

Great work @ShenpaiSharma !

@Qianqianye
Copy link
Contributor

Wonderful work @ShenpaiSharma and @calebfoss 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple types of materials to support WebGL Geometries
6 participants