#1613 closed defect (fixed)
[PATCH] Better handle missing textures required by material/shader
Reported by: | historic_bruno | Owned by: | wraitii |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 19 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description
It seems the behavior is currently not well defined, as an example, the Carthage Italiote embassy as of r12499 is missing its specTex
but using a material with both specular mapping and self-lighting. There is no error or notification of that, only some very strange flickering behavior and random self-lighting state.
IMO the best thing is an error and refusal to load the model, so the problem is explicit and the artist can make the necessary change, rather than having it be covered up and forgotten.
Attachments (3)
Change History (15)
by , 9 years ago
Attachment: | samplerCheck.patch added |
---|
comment:2 by , 9 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 19 |
Summary: | Better handle missing textures required by material/shader → [PATCH] Better handle missing textures required by material/shader |
follow-up: 4 comment:3 by , 9 years ago
I dont think refusing to load the model is a good thing as written in the description is a good thing. It would be really weird for newbies. I really support your patch if its the cleanest way to do it.
Could you try to remove a texture on an actor with your patch and see if units demo returns any error messages ?
Also, I think it should have a trigger option like SM31 debug warnings to avoid unnecessary performance problems.
comment:4 by , 9 years ago
Replying to stanislas69:
I dont think refusing to load the model is a good thing as written in the description is a good thing. It would be really weird for newbies.
Only if they make a mistake, this will never happen if the actors and materials are used properly. It doesn't sound weird for the game to reject broken content :)
by , 9 years ago
Attachment: | BetterPatch.patch added |
---|
follow-up: 10 comment:6 by , 9 years ago
New version of the patch is imo commitable. It adds the "define" thing. I removed the check for "sampler given but not required" because I realized that materials falling back to "simpler" versions cause a lot of those warnings for no good reason.
Incidentally I tried with the full graphical settings and it turns out some actors are wrong:
ERROR: Actor art/meshes/props/hele_stoa2_trees.dae: required texture sampler aoTex not found (material art/materials/basic_trans_ao.xml) ERROR: Actor art/meshes/structural/maur_wall_medium_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_gate_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_long_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_short_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_tower_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml)
by , 9 years ago
Attachment: | BetterPatch-v2.patch added |
---|
comment:7 by , 9 years ago
C++11 version following comments by Leper (which tbh were very very vague).
I'm not sure how we should handle lambdas for readability?
follow-up: 9 comment:8 by , 9 years ago
Does it work the other way other round ? Saying that textures are provided but material is not using them ?
comment:9 by , 9 years ago
Replying to stanislas69:
Does it work the other way other round ? Saying that textures are provided but material is not using them ?
I'm not sure that should be checked by the engine, maybe a warning but would there be any benefit? On the other hand, the reference checker script might be changed to do just that :)
comment:10 by , 9 years ago
Don't forget to update MaterialSystem once the patch is committed.
I don't fully understand the reason for combining the new <required_texture name>
tag with the existing defines - is it just to make the material files a bit neater?
Replying to wraitii:
Incidentally I tried with the full graphical settings and it turns out some actors are wrong:
ERROR: Actor art/meshes/props/hele_stoa2_trees.dae: required texture sampler aoTex not found (material art/materials/basic_trans_ao.xml) ERROR: Actor art/meshes/structural/maur_wall_medium_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_gate_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_long_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_short_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml) ERROR: Actor art/meshes/structural/maur_wall_tower_palisade.dae: required texture sampler specTex not found (material art/materials/player_trans_ao_parallax_spec.xml)
Hmm, those messages are misleading, it is referring to meshes as actors, which is not the case. For those cases it might be obvious which actor corresponds to which mesh, but since they can be shared, that may not always be the case. It would be ideal to name the actor.
comment:12 by , 8 years ago
Keywords: | review removed |
---|
Stan talked about this on IRC yesterday and I figured there would be a semi-easy way of doing this.
The attached patch adds a new option for materials: adding a "required_sampler" item that says which textures the actors must provide. It gives error on loading (still loads the mesh) if unneeded samplers are provided and if required samplers are not provided. Note that currently, loading "Units Demo" shows no error so at least entities are fine. Some actors may not be.
Note that this patch is doing this really inefficiently (I create a lot CStrInterns and my looping cjeck is at best bad) but I'm submitting it as there's a simple design issue here and it works. If the implementation is fine by people I'll commit a cleaner version.
The design issue: Currently whether a material uses e.g. normal maps or not is defined in the line
I think we could add that as an optional argument to the "required_sampler" instead (not removing "define" entirely, just removing those as they are basically redundant) to have something like
However those defines would be unnecessary if materials used a different technique. So it's really depending on whether we want to keep the current system with a lot of defines or multiply the number of technique files (which is strictly equivalent engine-wise).