in the latest CVS panda, the team PandaSE in ETC made a flat color bug fix and added a small feature that make auto shader customizable now. You can still use the default setShaderAuto() to turn on all the auto shader features panda can support, but you can also turn off certain feature for performance or visual effect concern. The switches we made for customization include (by order):
For example, if you only want to turn on normal map(without gloss map), you can use setShaderAuto(“norrmal-on”,"","","",""). Originally we try to implement the switch in bitmask way, considering the scripting readability, we choose this string way to make these switches.
Furthermore, if you tried to export a model with a Phong material in MAYA previously, the exporter wouldn’t let you use flat color. It would always show up gray even if you chose a specific color for the model, but didn’t use an image file as a texture. Now this bug is also fixed.
We hope you try it and give us some feedbacks.
Wei-Feng Huang
Graduate Student
Entertainment Technology Center
Carnegie Mellon University
Pittsburgh, PA
I find this to be terribly suboptimal for several reasons:
set_shader_auto() is a switch to set automatic shading on. If you would have, for example, added a function like set_effect() that would take an effect as the first parameter and a boolean as the second, then it would be much less confusing. The API would not be so polluted as with the current approach.
Strings? That’s a terrible idea for two reasons: a) they waste RAM; b) they are open for human error. Going with your approach of adding parameters to set_shader_auto(), booleans would have worked just as well with a descriptive parameter name for each (e.g. set_shader_auto(normal=true, glow=false)) with much less RAM waste. Imagine someone having to remember “normal-on” instead of just normal=true or normal=1. Makes much more sense that way.
Another approach, since you’re focusing on exclusion of certain effects, could be adding a disable_effect() with an enumerated type variable as the parameter (which would still be just as readable and not waste RAM). Same with an enable_effect() function.
All of this can be discussed with the other core developers of the engine of course. I’ve said my opinion.
I dislike this choice of API for the following reasons:
It is more error-prone for the fact that it is possible to make typos that silently break it. To prove my point; have you noticed that you typed “norrmal-on” in your original post (double r)?
Not only that, but it is also possible that someone accidentally specifies “normal-on” in the second field instead of the first field, and it would not work as expected.
With this design, the ABI breaks whenever a feature is added that should be toggled on/off easily. That means we are restricted to changing anything regarding this to major releases only.
It does not match Panda3D’s existing core API design. Not just because set_shader_auto was meant to be like set_shader_off, but it is totally unconventional to use the nuisance of strings for this purpose. A much better approach would be using bitmasks (an enum could be employed to make it easier to use), but that still wouldn’t be perfect because of the following reason:
Why limit the interface to toggling only these features? There are hundreds of different attributes that a node can have. It is way too monolithic to restrict this functionality to only normal, gloss, glow mapping and light ramps/shadows.
The interface is not intuitive to people that know how the Scene Graph works. The interface should make it less ambiguous as to how this kind of thing would be handled when child nodes override these values. How would a child node re-enable such a value, or disable a certain feature? And how would I specify an auto-shader attribute on a child node in a way that it doesn’t affect the settings of a parent node?
Furthermore, we already have a way to turn off gloss maps and such: just remove your gloss map from the texture state, using setTextureOff() or clearTexture().
The scene graph is already designed with a mechanism to fully specify the desired render state. I’m not sure about the value of an additional switch to disable certain properties.
So if the user disables normal mapping in the in-game settings we should traverse the entire scene graph removing textures? and then hope we don’t accidentally read one in later on? having an easy to use off switch sounds like a useful feature to me.
as to the other issues, I’m not experienced enough to comment, but this sounds like a generally good idea, whether or not the implementation could be improved.
No, the scene graph is already a hierarchical state-manipulation tool. If you want to turn off a normal map globally, you should only need to do:
render.setTextureOff(myNormalMapTextureStage, 1)
and the appearance of myNormalMapTextureStage in the scene graph, now and in the future, will be suppressed.
In practice, there are some issues with this, which our friends at CMU have already identified to me, but these issues are relatively minor, and I will address them shortly.
But the point is that Panda’s scene graph design is intended to avoid the need for highly-specific local changes like this.
Based on the feedback we have got on this thread, we have modified our original idea of using strings for such a feature into a single bitmask integer.
It’s the end of the semester, so I thank you for all your help through this semester to the PandaSE student team, and we are happy to hear that our features will be useful to the community. The limitation of this particular feature as David pointed out is that it’s a local solution and enhancing the set_texture_off() function is the most optimal way of doing it. This will be done by me or some other project here at CMU.
Isn’t that what we were talking about earlier? That is, that you should be able to do:
render.setTextureOff(myNormalMapTS, 1)
and it should remove your normal map from the state, with no need to have a special-case behavior in the shader generator.
The “bug” was not a bug per se, but rather a design flaw, in that the above call would actually disable all textures, due to the override, which is applied to the TextureAttrib and thus overrides all setTexture() operations.
I’ve just corrected this design flaw, so that the override on a setTexture() or setTextureOff() operation applies only to the indicated TextureStage. So now the above setTextureOff() call works as expected: it will disable the named TextureStage only.
ah, I am sorry. I was confused when you said it was a bug.
And yes, this makes the switch on the auto shader generator rather unnecessary. I will try it out and probably rollback on the shader generator behavior.
