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

Objects with ambient material only show color using ambientLight #6022

Closed
17 tasks
spaderkung opened this issue Feb 15, 2023 · 27 comments · Fixed by #6053
Closed
17 tasks

Objects with ambient material only show color using ambientLight #6022

spaderkung opened this issue Feb 15, 2023 · 27 comments · Fixed by #6053

Comments

@spaderkung
Copy link

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

No response

Web browser and version

No response

Operating System

No response

Steps to reproduce this

If this is intentional, it is not explained in the online reference. specularMaterial shows color.

Steps:

  1. Create WEBGL canvas
  2. Create a camera
  3. Make a colored ambientMaterial(r, g, b)
  4. Make a white directionalLight(x)
  5. Only grayscale visible on 3D shapes.

Snippet:

let cam
function setup() {
	createCanvas(windowWidth, windowHeight, WEBGL);
	
	cam = createCamera()
	camera(0, -100, 300)
}

function draw() {
	background(230);
	let ls = 255
	ambientMaterial(0, 255, 0)
	directionalLight(ls, ls, ls, 0, -100, -300)
	rotateY(millis()/2000)
	box(50)
}
@spaderkung spaderkung added the Bug label Feb 15, 2023
@welcome
Copy link

welcome bot commented Feb 15, 2023

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

@davepagurek
Copy link
Contributor

This is intentional, we can mention in the docs that ambientMaterial only affects the color of the ambient light portion of an object's lighting.

@spaderkung
Copy link
Author

spaderkung commented Feb 15, 2023 via email

@nikhilhvr
Copy link
Contributor

I'd like to contribute to this issue.
Let me know if you're also working on this. Excited to contribute!

@davepagurek
Copy link
Contributor

@nikhilhvr I think you can start working on this if you're interested! Let me know if I can help clarify anything as you go.

@nikhilhvr
Copy link
Contributor

@davepagurek I would love to try my hands please guide me on how to get started.

@davepagurek
Copy link
Contributor

If you go to a page on the p5 reference that you want to edit, e.g. the ambientLight reference, you can scroll down to the bottom and there will be a link to the source code for both the function itself and its documentation:

image

That particular reference page links to this: https://github.com/processing/p5.js/blob/v1.6.0/src/webgl/light.js#L11

If you want to edit the description or add an example, you can do so in that block comment.

@Tapo41
Copy link

Tapo41 commented Mar 4, 2023

I want to work on it please assign it to me if it is still available

@davepagurek
Copy link
Contributor

I'll just double check first with @nikhilhvr -- @nikhilhvr, are you looking into this one now? If so, I can assign it to you! Otherwise, no worries, it looks like @Tapo41 is interested so we can assign this to them 🙂

@nikhilhvr
Copy link
Contributor

Yes, @davepagurek I am still working on this issue.
I would try to pull request few examples from today onwards.

@davepagurek
Copy link
Contributor

thanks, I'll assign this to you!

nikhilhvr added a commit to nikhilhvr/p5.js that referenced this issue Mar 5, 2023
@nikhilhvr
Copy link
Contributor

hey @davepagurek can you review my PR.

@Qianqianye
Copy link
Contributor

Thanks @nikhilhvr for working on it. Before we make more edits in the pull request, I believe it'd be helpful to discuss the example code and documentation we'd like to add to the ambientLight reference in this issue first.
I think the example code @nikhilhvr started on the pull request is a great starting point, but it currently only included directionalLight.

Would it make sense to include ambientLight in the example code? Like two boxes with same ambientMaterial, but one with ambientLight and another with directionalLight?

@nikhilhvr
Copy link
Contributor

Thanks, @Qianqianye for the suggestion I will consider it.

@nikhilhvr
Copy link
Contributor

Hey, @Qianqianye I made some enhancements in the example Can you please tell me if this is okay or if I have made other changes?
image

@davepagurek
Copy link
Contributor

Thanks for your work on this so far @nikhilhvr! How do you feel about maybe trying the value of ls to e.g. mouseY so that one could move the mouse up and down to see the difference between full white ambient light and no ambient light?

@nikhilhvr
Copy link
Contributor

Thanks, @davepagurek helping me to reach so far. I am really grateful you mentored me.

Code

let cam
  function setup(){
  createCanvas(100,100,WEBGL);

  cam =createCamera();
  camera(0,-100,300);
}
  function draw(){
  background(230);
  ambientMaterial(0,255,0); //Green Material
  ambientLight(0,mouseY,0,0,-100,300); 
  //As you move the mouse up to down it changes no the ambient light to full ambient light.
  rotateY(millis()/2000);
  box(100);
}

Working

P5.JS.mp4

@davepagurek
Copy link
Contributor

Looks great @nikhilhvr! It might be worthwhile to use the same value for each channel of ambientLight to show that white ambient light brings out the ambient material without any tint and one does not need to make the ambient light exactly match the ambient material.

@nikhilhvr
Copy link
Contributor

Hey, @davepagurek I didn't get what I suppose to do further can you please help me out?

Looks great @nikhilhvr! It might be worthwhile to use the same value for each channel of ambientLight to show that white ambient light brings out the ambient material without any tint and one does not need to make the ambient light exactly match the ambient material.

@davepagurek
Copy link
Contributor

Sure thing! In an earlier version of your code, you had ambientLight(ls, ls, ls), meaning the red, green, and blue channels of the ambient light all get the same value (this is what I mean when I say "white" ambient light.) This means that an object with a green ambient material will appear green, but with adjusted brightness, and also an object with a red ambient material will appear red with adjusted brightness.

In your most recent video, you have ambientLight(0, mouseY, 0), so the ambient light only has any value in the green channel. For an object with green ambient material, it will look the same as before, but for ab object with a red ambient material, the object would look black.

My suggestion is to use the same value for each channel so that viewers of the example can update the ambient material and still see the effects of changing the ambient light. If we tint the ambient light by making it green, then viewers would also have to modify the ambient light in addition to the ambient material.

@nikhilhvr
Copy link
Contributor

Thanks, @davepagurek for this detailed explanation.

Code update

let cam
  function setup(){
  createCanvas(100,100,WEBGL);

  cam =createCamera();
  camera(0,-100,300);
}
  function draw(){
  background(230);
  ambientMaterial(237,34,93); //Pink Material
  ambientLight(0,mouseY,0,0,-100,300); 
  //As you move the mouse up to down it changes from no ambient light to full ambient light.
  rotateY(millis()/2000);
  box(100);
}

Quick code demo

p5.jss.online-video-cutter.com.mp4

@davepagurek
Copy link
Contributor

The code in your video looks good @nikhilhvr! (Just a heads up, it looks like the code above the video is different, as it doesn't repeat mouseY.)

Also, I think you only need three parameters to ambientLight for the three color channels -- after your mouseY parameters, you don't need any more.

@nikhilhvr
Copy link
Contributor

nikhilhvr commented Mar 17, 2023

Hey, @davepagurek is there anything thing left that I can change into this code?

let cam
  function setup(){
  createCanvas(100,100,WEBGL);

  cam =createCamera();
  camera(0,-100,300);
}
  function draw(){
  background(230);
  ambientMaterial(237,34,93); //Pink Material
  ambientLight(mouseY); 
  //As you move the mouse up to down it changes from no ambient light to full ambient light.
  rotateY(millis()/2000);
  box(100);
}

@davepagurek
Copy link
Contributor

Looks good @nikhilhvr! I think we just need to update the indentation to be consistent (de-indent the function lines, spaces on both sides of the = in createCamera) and maybe be consistent with semicolons at ends of lines) but I think the functions being called are great now.

Feel free to update your PR with this 🙂

@Qianqianye
Copy link
Contributor

Thanks @nikhilhvr and @davepagurek for working on the example code.
Please correct me if I am wrong, do we need to declare variable cam in this example? Not sure if we need the two lines let cam and cam =createCamera(); .

@davepagurek
Copy link
Contributor

Good catch @Qianqianye! Agreed, I think the camera(0, -100, 300) is sufficient here without making another camera.

@nikhilhvr
Copy link
Contributor

Thanks, @davepagurek @Qianqianye for the suggestions. I am done with enhancements and also updated the PR.

  function setup(){
  createCanvas(100,100,WEBGL);
  camera(0,-100,300);
}
  function draw(){
  background(230);
  ambientMaterial(237,34,93); //Pink Material
  ambientLight(mouseY); 
  //As you move the mouse up to down it changes from no ambient light to full ambient light.
  rotateY(millis()/2000);
  box(100);
}

davepagurek added a commit that referenced this issue Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@davepagurek @spaderkung @Qianqianye @nikhilhvr @Tapo41 and others