Use _Color and _MainTex instead of _BaseColor and _BaseMap

[Holo-Krzysztof]

This PR renames 2 shader properties so they're accessible via common C# accessors as described in https://github.com/atteneder/glTFast/issues/386.

Originally I did the change in shader graph, but then noticed that the serialization format in 10.8.1 is very different from the files in this repo, so I went back and made the changes in a text editor. It fixes the issue for Unity 2020.3.31 and Shader Graph 10.8.1, though it probably breaks newer ones instead as I did not change the non-legacy shader graphs in any way.

Our use case is that we have a shader that can do selection (like mouse picking) in XR without colliders. After importing we change shaders and colors are lost because material properties have different names than e.g. the Unity URP Lit shader.

[atteneder]

@Holo-Krzysztof Thanks for this PR and reporting #386 !

Although generally I'm in favor of making this change, it's a breaking change and requires a major version bump. glTFs that were imported with older (the current) versions will break, so for this to land we first have to:

• Make sure existing glTFs get re-imported automatically when upgrading
• Make sure updating to newer SRPs works by applying same changes to the non-legacy shader graphs as well
• Testing, testing, testing!

I'm currently preparing a new release with more breaking changes, so it might fit in.

[Holo-Krzysztof]

Sounds good! I can help with testing within our project and Unity/SRP combination. We want to use glTFast for runtime import, but there's also some assets for 3D launchers for HoloLens which are gltf, so could cover the editor import case too.

[atteneder]

Update: When thinking about glTF material import in the bigger picture, three approaches come to mind:

1. Import to glTF shaders (current default)
2. Import to Unity Lit/Standard shaders (to be implemented; see #258)
3. Custom material import (i.e. custom IMaterialGenerator implementation)

For your specific use-case I'd recommend approach number 3, but if you want to stick to your solution, having a shader with close-to-identical properties to Lit/Standard makes sense. As you can guess by now, solution number 2 would be the next best thing (sorry it's not implemented yet).

The glTF shaders' properties can be modeled on...

• ...the Unity Lit/Standard properties (which is what you pursue with this PR)
• ...the glTF material properties (including extensions' properties)

I got some more thought input from @hybridherbst and there are many good reasons to model the glTF shader properties on the glTF material model. For example, if you make your own shader (solution 3 from above), you just have to match the glTF material properties and your 99% done and don't have change much of the existing IMaterialGenerator implementation. So as it stands now, I'm in favor of changing the properties towards the glTF material spec.

Happy to get your thoughts and discuss.

Thanks

[Holo-Krzysztof]

2. would actually be the best thing in my case I think, because we have to include the URP Lit shader in our app for some other purpose already. Is that something you're planning for the near future?

Writing a custom material instantiator is something I'd like to avoid for now, because that selection shader we use is quite complex (inherited from previous team) and I'm not familiar with it at all. I could probably ask a colleague to help me out, but it'd be a bit of work whereas changing material properties basically fixes the current issue already.

[hybridherbst]

I think it's worth noting that (2) is at best a lossy conversion, URP/Lit does not have the same features as glTF PBR + Extensions.

And especially at runtime, (2) will have worse load time and potentially memory performance (as textures need to be unpacked/reshuffled/things converted).

[Holo-Krzysztof]

I think we could live with some minor inaccuracies (at least for the HoloLens build), but worse import performance also doesn't sound ideal. :/

Also, I just checked and the selection shader is basically a URP shader graph based on the Lit material, so I guess whatever we do, we need to import to that anyway in the end, am I understanding this correctly?

How big of a performance impact are we talking about with the texture conversion stuff?

[hybridherbst]

Just for my understanding, why are you not refactoring the other way around and have the selection shader that's used for glTF objects match the properties as they are imported? That will certainly give you more flexibility / less headache...

(there's tools to refactor shader properties across a project if that's a concern here)

[Holo-Krzysztof]

Because we also support other formats (obj, fbx, jt with more to come in the future) and those might use e.g. the Lit shader properties at import time, so we'd need the selection shader to handle both in the end. For example, we use TriLib for fbx right now and that maps to the URP lit material if I remember correctly. I think Unity's USD implementation does that as well.

[hybridherbst]

I'd recommend you have multiple selection shaders then. The memory impact is considerable, especially when targeting effectively mobile devices. Neither FBX nor USD are good delivery formats – they end up consuming way more memory. I understand that you want to generally support them in a multi-purpose viewer but then you/your users are missing out on the advantages of glTF as dedicated transmission format, especially when used with the right compression settings.

To illustrate, here's a simple textured quad with a 4k texture, once with proper GPU texture compression and once without: TexturedQuadMemory.zip In terms of file size, these are tiny - but throw them into gltf.report and you see what happens on the GPU:

TexturedQuad

TexturedQuad.etc1s

The latter uses 4x less GPU memory, super important in an already constrained environment. If this had an ORM map applied, and that has to be unpacked, you're at 8x more memory than using a "clean" glTF import path with the advantages that gives. The latter also loads considerably faster since it doesn't have to upload so much data to the GPU and doesn't have to unpack PNGs into RGBA32.

[Holo-Krzysztof]

That sounds like a lot. Hm. I'll be talking to my colleagues about the effort required to get the selection functionality into the glTF shader graphs, maybe it won't be too bad. But wait, wouldn't we need to modify the glTFast package to do this? Like, if we want a shader that can use glTF materials + extensions + selection? (I'm not too familiar with shader graph). Or is it possible to tell glTFast to use a particular shader from outside the package?

Until now GLTF was not really a format we recommended a lot since we used a fork of the Khronos Unity lib hacked together to provide non-blocking import, but that was many years ago, before glTFast even existed. In general we want to offer better interop between our app and others and the formats we're considering are GLTF and USD since they have the fastest import times by far; it kind of depends on what either can do (import/export features from Unity) and what the customers want.

I'll be talking to people and come back to this issue as soon as I know more. Thanks for your responses so far. :)

[Holo-Krzysztof]

As a temporary workaround, I copy the property values from the glTF shader graph and apply to the corresponding ones in our shader. It's not super accurate PBR-wise, but I don't need to fork the package (can actually use 4.6.0 for the release) and it's something that we didn't have before either, so at least not regressing while getting considerably faster import.

Doing this correctly would require more time which we don't really have at the moment. We'll probably revisit this as soon as we know more about the interchange format we decide on and at that point having PBR-correct rendering/materials will be more of a priority.

I think you can close this for now. Thanks for your time.

[atteneder]

As discussed earlier, your use-case is very valid, but stands in contradiction to normalizing glTF shader property names.

I finished said renaming work now and it landed in the main branch effectively breaking this PR, so I'm closing it now.

Thanks for your inputs.