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

WebGL ambientMaterial() defaults to white instead of the fill color #6125

Closed
1 of 17 tasks
davepagurek opened this issue May 1, 2023 · 4 comments · Fixed by #6130
Closed
1 of 17 tasks

WebGL ambientMaterial() defaults to white instead of the fill color #6125

davepagurek opened this issue May 1, 2023 · 4 comments · Fixed by #6130

Comments

@davepagurek
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build Process
  • Unit Testing
  • Internalization
  • Friendly Errors
  • Other (specify if possible)

p5.js version

1.6.0

Web browser and version

Firefox 108.0

Operating System

MacOS 12.5.1

Steps to reproduce this

Here's an example sketch that shows how the colors in WebGL mode changed between v1.4.2 and the current version, and how to get back to the same look under the current system:

// v1.4.2
fill(this.c)
// v1.6.0
fill(this.c)
// v1.6.0
const c = color(this.c)
ambientMaterial(
  red(c),
  green(c),
  blue(c)
)
fill(this.c)

This happened when we made it possible to separately tweak the ambient material from the specular or fill material of an object. We did this because Processing also allows this, but Processing also defaults the ambient material to the fill material unless explicitly specified.

To match the default behaviour of 1.4.2 (and also Processing) to not get washed out colours by default, we should also default the ambient material to the fill material unless an ambient material is specified.

@inaridarkfox4231
Copy link
Contributor

If the vertex color is used, the color determined by fill is not used, but what should I do in that case?
Also, I don't think it's a very clean process to just specify the same color.

Also, this sketch assumes the use of lights(), but I don't think it's appropriate to disregard that assumption. Is there any problem with the specification of lights()? I don't think it's a simple problem.

Emphasizing that this is not a right or wrong answer, we do this instead of lights().

directionalLight(255,255,255,0,0,-1);
ambientLight(48);

255_48

The problem of appearance is really difficult because subjectivity is involved. I think that some coders may not mean it because the appearance is determined arbitrarily just by not executing ambientMaterial().

@davepagurek
Copy link
Contributor Author

If the vertex color is used, the color determined by fill is not used, but what should I do in that case?

The default would be to use the same material color for everything, so I think in this case the ambient material would be the vertex color. You bring up a good point though about it being hard to tweak the ambient colour for vertex colored materials, although I'll maybe discuss that more in your other issue, as this one is more of a bug fix to get back to Processing's behavior.

Also, I don't think it's a very clean process to just specify the same color.

Agreed, so fixing this issue would mean that you wouldn't have to, as it would default to the same color.

Also, this sketch assumes the use of lights(), but I don't think it's appropriate to disregard that assumption. Is there any problem with the specification of lights()? I don't think it's a simple problem.

lights() is just a shortcut for some specific directional lights and ambient light values, so this bug fix would work the same for both.

@inaridarkfox4231
Copy link
Contributor

I still don't quite understand what #5774 means, but it seems to boil down to using ambientMaterial instead of fill when using lighting method...

I felt that way because the sample code doesn't use fill.
Also, in the example work, if fill(this.c) was set to ambientMaterial(color(this.c)), the appearance certainly changed.

However, it is natural to use fill() when coloring with a single color, so I feel that there is a misunderstanding and that is causing the problem.

@davepagurek
Copy link
Contributor Author

Just put up a PR making the behaviour match Processing! It should restore the earlier behaviour when just using fill() without specifying any other material properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: DONE! 🎉
Development

Successfully merging a pull request may close this issue.

2 participants