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.
Entertainment Technology Center
Carnegie Mellon University
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?
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.
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:
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.