Regarding the incompatibility between texture coloring and specular, and the new flag about ambientMaterial

[inaridarkfox4231]

Topic

First of all, I don't know why this is the case, but I would like to raise an issue.

Regarding the fact that texture coloring and specular cannot be combined

Currently, using texture coloring with specularMaterial() does not reflect specular.

let tex;

function setup() {
  createCanvas(400, 400, WEBGL);
  tex = createGraphics(256, 256);
  tex.noStroke();
  for(let i=0; i<256; i++){
    tex.fill(i, i, 255);
    tex.rect(0, i, 256, 1);
  }
  noStroke();

  background(0);
  lights();
  specularMaterial(255);
  shininess(400);
  texture(tex);
  //fill(255,0,0);
  plane(200);
}

currentOutput / expectedOutput

texture_specular

Ignore the color fading problem for the time being (it was resolved in a recent pull request, but...)

This is because texture() turns the specularMaterial flag off inside the function (it also truns the emissiveMaterial flag off).

p5.prototype.texture = function(tex) {
  this._assert3d('texture');
  p5._validateParameters('texture', arguments);
  if (tex.gifProperties) {
    tex._animateGif(this);
  }

  this._renderer.drawMode = TEXTURE;

  this._renderer._useSpecularMaterial = false; // turn off
  this._renderer._useEmissiveMaterial = false; // turn off

  this._renderer._useNormalMaterial = false;
  this._renderer._tex = tex;
  this._renderer._setProperty('_doFill', true);

  return this;
};

I don't know why they don't make it compatible, but it doesn't seem to be a problem even if it's compatible within the shader, so I don't think it's necessary to fold it (If it's folded for a reason, I left it as it is. I think it's better...).
In addition, specularMaterial(), ambientMaterial(), and emissiveMaterial() all set _tex to null, but I don't think this is necessary either. This is because they are compatible within the shader without contradiction.

p5.prototype.specularMaterial = function (v1, v2, v3, alpha) {
  this._assert3d('specularMaterial');
  p5._validateParameters('specularMaterial', arguments);
  var color = p5.prototype.color.apply(this, arguments);
  this._renderer.curSpecularColor = color._array;
  this._renderer._useSpecularMaterial = true;
  this._renderer._useNormalMaterial = false;
  this._renderer._enableLighting = true;

  this._renderer._tex = null; // turn off

  return this;
};

The question of whether fill() and texture() should set _hasSetAmbient to false

In #6130 a flag _hasSetAmbient was created. This is true once ambientMaterial() is called and there is no way to make it false. Also, if false, the color specified by ambientMaterial() will be the baseColor.
But I think it's better to turn off this flag when fill() and texture() are called.

First, currently the way to enable per-vertex coloring is to call fill(). Also, the way to enable texture coloring is to call texture().
If the _hasSetAmbient flag is set, and you want ambientMaterial() to set the same color as fill(), this can be achieved by specifying the same color as fill() was called with. However, in the case of vertex coloring and texture coloring, this method cannot be used, and the same color cannot be used without breaking the flag.

If you want the color determined by fill() or texture() to be compatible with the color determined by ambientMaterial(), you can achieve this by calling ambientMaterial() after fill() or texture (for that, ambientMaterial( ) should not cause _tex to be null...).

Also, when drawing only with lighting processing and ambientMaterial() without using fill() or texture(), the flag remains set and there should be no particular problem (in this case, the color determined by fill() is remains white).

Summary,

• texture() stops setting _useSpecularMaterial and _useEmissiveMaterial to false
• ambientMaterial(), specularMaterial(), emissiveMaterial() stop nullifying _tex
• Make _hasSetAmbient false when fill() and texture() are called

I would like to discuss these.

[davepagurek]

For whether or not fill() and texture() should reset the ambient material, I think it runs into an issue with ordering: if fill() sets _hasSetAmbient = false, then fill(color1); ambientMaterial(color2) leads to a different result than ambientMaterial(color2); fill(color1). I implemented it without setting it to false right now because that's what Processing chose to do, but I think it's OK to deviate from that as long as we document our choice.

Are you imagining that fill(color) would do _hasSetAmbient = false but specularMaterial(color) would not? I think that could be a good implementation, the rule would be: use fill to set all the properties at once; use specularMaterial to just set that one part.

I think it would be great to allow the use of texture() with specularMaterial()! It runs into a similar ordering issue: texture(img); ambientMaterial(color) would work differently from ambientMaterial(color); texture(img). Do you think that's OK? We could maybe also let you call specularMaterial(img) to set just the texture without overriding the ambient color to be consistent with fill/specularMaterial.

[inaridarkfox4231]

For whether or not fill() and texture() should reset the ambient material, I think it runs into an issue with ordering: if fill() sets _hasSetAmbient = false, then fill(color1); ambientMaterial(color2) leads to a different result than ambientMaterial(color2); fill(color1). I implemented it without setting it to false right now because that's what Processing chose to do, but I think it's OK to deviate from that as long as we document our choice.

The problem is that the way to switch texture coloring to per-vertex coloring is the act of calling fill() pointlessly. Since there is no function like enablePerVertexColoring(), the only way to set the flag is to call it like fill(0). Regarding the order, I think that there is no problem if I add it to the reference.

In the first place, it was unexpected for me that ambientMaterial() and specularMaterial() would make _tex null. This is exactly the case when it comes to ordering issue, but it's not written anywhere in the reference. Rather than adding this, I think it would be better to stop making _tex null in the first place. The texture is used only to determine the baseColor within the shader, and is completely independent of processing such as specular. It's also puzzling that texture() turns off the flag to use specular. I think the two should be able to coexist naturally.

I think it would be great to allow the use of texture() with specularMaterial()! It runs into a similar ordering issue: texture(img); ambientMaterial(color) would work differently from ambientMaterial(color); texture(img). Do you think that's OK? We could maybe also let you call specularMaterial(img) to set just the texture without overriding the ambient color to be consistent with fill/specularMaterial.

If texture() stops turning off the flag for using specular, etc., and functions such as specularMaterial() stop setting _tex to null, the ordering issue does not occur, but oversight and If you have a serious problem, I would appreciate it if you could let me know.

[inaridarkfox4231]

Personally, I think the reason why texture and specular cannot coexist is that there was an oversight in #5774 when specularMaterial() etc. was changed so that colors can be specified individually. From start to finish, that pull request didn't consider alternative coloring methods such as textures at all. It seems very natural for texture coloring and lighting to coexist, but I wonder why specular can't be used. If you only use fill(255) and only use ambientMaterial(), you can only do monochromatic colors, but I think it's inconvenient.

[davepagurek]

I think that change makes sense. For fill/specularMaterial, we should just look through the docs and:

• make sure examples using fill/specularMaterial look the way they intend, and update them if they don't
• add to the fill and texture documentation explaining this behaviour
• maybe add an example to ambientMaterial combining it with specularMaterial to show a more practical example of how it's used? The current examples all have flat colours, maybe we can make a p5 version of this Processing example so we actually have one with shading

[inaridarkfox4231]

make sure examples using fill/specularMaterial look the way they intend, and update them if they don't

The only spec change I'm proposing for fill() is to set _hasSetAmbient to false when it runs. So if there is a problem, it will be when used together with ambientMaterial().
If you run ambientMaterial() after fill(), it's conventional. If not executed, uAmbientMatColor will contain the same color as determined by fill(), but if there was a problem with this, it would have occurred in #6130, so there is no problem.
When coloring using fill() after ambientMaterial(), the result will be different from before, but I think it would be better to prepare a new unit test.

As for specularMaterial(), I'm going to stop nulling _tex, but there probably isn't an example of using this with texture, so I'm going to have a unit test to prove that it works too.

add to the fill and texture documentation explaining this behaviour

for fill(): "If you execute this, the ambientColor will be the same color that was determined at that time, but if you want to determine the ambientColor yourself, you must then execute ambientMaterial() before drawing.''
for texture(): "After doing this, the ambientColor will be the same as the texture color determined by this function, but if you want to determine your own ambientColor, then you must call ambientMaterial() before drawing."

maybe add an example to ambientMaterial combining it with specularMaterial to show a more practical example of how it's used? The current examples all have flat colours, maybe we can make a p5 version of this Processing example so we actually have one with shading

If ambientMaterial and specularMaterial coexist, I think there is already an example at specularMaterial(), but I think it makes sense if I create an example that is not monochromatic.
Should I add the vertex coloring example and the texture coloring example coexisting with specularMaterial() to the specularMaterial() reference...?

[inaridarkfox4231]

I would like to write it like "ambientColor (color determined by ambientMaterial())" because it is easier for the reader to understand (because it is difficult to understand with the variable name in the shader).

[inaridarkfox4231]

I'll create a pull request for now. I'm not sure why textures aren't allowed to coexist with specular, so I'll run it through unit tests and see what happens.

[inaridarkfox4231]

This is a comparison video. There are three versions: the current version, the latest version, and the version with this specification change applied. For each of them, I changed the order of calling texture() and specularMaterial() and checked them in two ways.

current version

No texture is applied, or the color is faded when applied.

fujisan0.mp4

latest version

The color fading problem is gone, but no specular is applied.

2023-05-09.20-43-55.mp4

the version with this specification change applied

The colors are not faded and the specular is applied neatly out of order.

2023-05-09.20-44-23.mp4

[inaridarkfox4231]

I wanted to raise it to a pull request, but it didn't work for some reason, so I put it here. I'm sorry.

[inaridarkfox4231]

There is no way to deal with it because it does not respond at all.
I will give up on this issue. I have two more suggestions to make, so I'm going to open another issue. I'm a little tired.

As long as ambientMaterial() isn't called, the color won't fade, so that's fine.

One of the suggestions is the texture part of this issue. After all, the process of setting _tex to null is incomprehensible, so I regard it as a bug and launch a new pull request just to solve it.

Another is to introduce the idea used in easyCam to orbitControl() to make the movement smoother. If this goes well, I think it will be a lot easier to operate.

[davepagurek]

Hi, sorry for the delay, I got sick on the weekend so I haven't been active on Github. It looked like tests passed on your PR after you made it, I can take a look at the code now if you're still interested!

[inaridarkfox4231]

Thank you for your reply. I've opened a pull request. Please leave a review. Take care.