Opened 9 years ago

Closed 6 years ago

Last modified 5 years ago

#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)

samplerCheck.patch (26.6 KB ) - added by wraitii 6 years ago.
BetterPatch.patch (38.7 KB ) - added by wraitii 6 years ago.
BetterPatch-v2.patch (39.6 KB ) - added by wraitii 6 years ago.

Download all attachments as: .zip

Change History (15)

by wraitii, 6 years ago

Attachment: samplerCheck.patch added

comment:1 by wraitii, 6 years ago

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

<define name="whatever" value="1">

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

<required_sampler name="norm_tex" define="USE_NORMAL_MAP" />

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).

Last edited 6 years ago by wraitii (previous) (diff)

comment:2 by wraitii, 6 years ago

Keywords: review patch added
Milestone: BacklogAlpha 19
Summary: Better handle missing textures required by material/shader[PATCH] Better handle missing textures required by material/shader

comment:3 by stanislas69, 6 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.

in reply to:  3 comment:4 by historic_bruno, 6 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 :)

comment:5 by stanislas69, 6 years ago

It's also non practictal to test X)

by wraitii, 6 years ago

Attachment: BetterPatch.patch added

comment:6 by wraitii, 6 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 wraitii, 6 years ago

Attachment: BetterPatch-v2.patch added

comment:7 by wraitii, 6 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?

comment:8 by stanislas69, 6 years ago

Does it work the other way other round ? Saying that textures are provided but material is not using them ?

in reply to:  8 comment:9 by historic_bruno, 6 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 :)

in reply to:  6 comment:10 by historic_bruno, 6 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:11 by wraitii, 6 years ago

Owner: set to wraitii
Resolution: fixed
Status: newclosed

In 16427:

Check when loading an actor that it defines all the texture samplers required by its material. Print out a readable error otherwise. Fixes #1613 (note that this does not check for unnecessary samplers as that is non-trivial and does not lead to graphical glitches).
Also add a shortcut for some defines to clean-up material files slightly and make the link between sampler and shader more explicit.

comment:12 by sanderd17, 5 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.