Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NORMAL_METALNESS should be specified in XPlaneLayer settings to avoid copy and paste problems, missmatch errors #357

Closed
danklaue opened this issue Jun 22, 2018 · 10 comments

Comments

@danklaue
Copy link

When exporting a layer filled with meshes that have a common material with a specular intensity of "1", and among these meshes is one that's got a material with a specular intensity of "0", many exported objects end up losing their specularity.

Expected result: only the objects with a material of specular intensity "0" should have an ATTR_shiny_rat of 0, and all other objects should retain their specular intensity of "1".

Actual result: even ONE object with a different specular intensity can screw up all other objects. That breaks WYSIWYG, as an object whose material is set properly in Blender may end up exported with a wrong shininess value, if another material is preset that trips the "ATTR_shiny_rat" attribute.

@bsupnik
Copy link
Collaborator

bsupnik commented Jun 22, 2018

Hi Dan,

We need a blender project we can export to reproduce this. We don't need any other analysis or for you to spend any time trying to tease out what has gone wrong - just a case where we can hit 'export' and observe non-WYSIWYG shininess because what's on the exported OBJs in X-plane doesn't visually match what was in Blender. Smaller test cases are better, but if you have a project that causes it, we'll take whatever you saw that made you report the bug.

@danklaue
Copy link
Author

OK, I created a test case, along with two exported objects. (See attachment).

ATTR_ShinyTestCase.zip

So, what we see here is, on layer 1 there's a scene where all objects have the same material. It exports fine, without even a mention of an ATTR_shiny_rat in the resulting .obj file.

On layer 2, however, I've copied the entire layer 1 scenario, but changed the material of one object to be non-PBR and non-shiny.

The resulting .obj file has only one ATTR_shiny_rat 0, which overrides all other objects' material settings.

Expected result: one would think that each object that has a material with "Normal Metalness" enabled and specular intensity set to 1 would export with "ATTR_shiny_rat 1", even if there's an object among them that has a material NOT enabled for Normal Metalness, and with specular intensity set to 0.

This is important, because sometimes you append a mesh from an older project, and if it's not set up like a PBR material, it can screw with the materials of the rest of the scene.

@bsupnik
Copy link
Collaborator

bsupnik commented Jun 26, 2018

@tngreene @danklaue I took a quick look at the test case and I think I see two bugs going on, neither of which are the same as the reported bug.

  1. this test case contains a mix of metalness and non-metalness materials - in both objects, and thus both exports should fail validation for mix-and-matching metalness. This is because the new v11 metalness work-flow is a whole-obj-level control and cannot be mixed mid-object.

@danklaue Is there a use case where you actively want to mix metalness and non-metalness? Or for that matter, a case where the material specular ratio should ever be set by hand when using metalness?

  1. Because the materials have no normal map, the metalness directive is never written to the object file. This is the issue Jim hit a few weeks ago where the metalness check-box seemed broken.

This is definitely a bit of an edge case, as the metalness work-flow without a normal map is pretty useless, but it's still astonishing behavior for the user. If it's not already, this should be a warning.

@danklaue
Copy link
Author

  1. OK, so technically, metalness should be an EXPORT setting (in "Layers", on the same level as the texture assignment), as opposed to on the MATERIAL setting.

Ben, as to your question: wouldn't really make much sense to mix and match. The moment you add a normal map, you're implicitly expressing an intention to go full PBR. Maybe in the slot where you define the "texture normal", you could put a checkbox that says, "Use PBR", and it'd make use of that normal map in the PBR specification. If the checkbox is OFF, it'd just use the normal map the way it used to be used in XP10. Also, if you were to check "Use PBR", you should assume that the author wants to use the normal map to assign all material settings, meaning the Global_Specular for that object should also be set to 1. Otherwise, you're left hunting for where in the world there's an object that's been exported with a glossiness of something other than 1.

  1. No idea what you're talking about there. I thought that when you enable the "Normal Metalness" checkbox, that's written into the .obj's header as "NORMAL_METALNESS", regardless of whether there's a normal map present or not. For me at least, that DOES seem like an edge case.

I'd have to draw up another test case, to see whether mixing and matching roughness values (and NOT mixing and matching Metalness) also produces the astonishing results... I THINK it did, but I'm not 100% sure now. (I.e., having all materials set to gloss 1, except for one, set to gloss 0.2, may cause all materials that come after that 0.2 to also adopt the value of 0.2, meaning a whole lot of objects that were actually assigned a 1.0 in Blender, are exported with 0.2)

@bsupnik
Copy link
Collaborator

bsupnik commented Jun 26, 2018

Hi Dan,

Re: the PBR check-box, we sort of already have that - the normal metalness check-box on your material. In the proposed fix, if these are on, everything works, if they're off, it's completely compatible, and if you mix and match we flag your materials as not making a ton of sense.

@tngreene
Copy link
Contributor

The bug @bsupnik is referring to is #353, btw

@danklaue
Copy link
Author

danklaue commented Jul 4, 2018

Mixing and matching materials isn't something that always happens on purpose. You sometimes import or copy-and-paste a mesh from a different project, or you have different materials assigned to night lighting variables.

My point is, if you can't mix and match metalness, you shouldn't be setting metalness on a per-material basis, no matter if you get flagged for doing so, it would make more sense to set metalness on a per-layer basis, as the entire layer will be exported as an object that either has or doesn't have the Metalness attribute.

It'd be better to change it so that metalness is NOT controlled in the materials, but rather, in the layers.

@tngreene
Copy link
Contributor

tngreene commented Jul 5, 2018

From a conversation with Ben (cleaned up): "While it is true that it is better to put data where it can't be duplicated than allow it to be duplicated with error messages, we've already done this for having the texture be on the material. This was chosen to match Blender's semantics rather than create our own."

In designing XPlane2Blender's interface and workflow there is a sliding scale between What We Like and How Blender Works. For instance, we could have made our Bone system out of checkboxes and empties, our own lamp system out of checkboxes and empties, our own material system out of textfields and checkboxes and inputs, and etc, but that would have been really confusing for people learning and using it.

On the otherhand extending Blender only when perfectly natural would stop us from building new features or create oddities like our current predicament here.

This is a design decision, which is better than a bike shed discussion but not as easy to solve. It is fortunately not a technical challenge.

@tngreene
Copy link
Contributor

tngreene commented Jul 5, 2018

Before continuing with talks about design decisions in another bug report: is there still a bug here? It appears that the resolution was that multiple materials were being used in the same layer with different metalness attributes, so not a bug, just not enough error reporting and potentially a wrong design decision.

@tngreene tngreene changed the title ATTR_shiny_rat not reset properly NORMAL_METALNESS should be specified in XPlaneLayer settings to avoid copy and paste problems, missmatch errors Oct 4, 2018
@tngreene
Copy link
Contributor

And so it shall be for v4.1.0-alpha! Thank you for filing this bug and reminding me about it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants