kurotu / VRCQuestTools

Unity editor extension to support uploading VRChat avatars for Oculus Quest
https://kurotu.github.io/VRCQuestTools/
MIT License
204 stars 14 forks source link

Auto-Remove Vertex Colors on by default may be harmful! #10

Closed valuef closed 8 months ago

valuef commented 1 year ago

Describe the bug VRCQuestTools has Auto-Remove vertex colors on by default in Windows projects. The effects of this that I have observed are that any mesh associated with an avatar descriptor will have its vertex color data stripped without notice.

For users who need to use vertex colors on their meshes, this default can be unwanted and destructive.

To Reproduce Steps to reproduce the behavior:

  1. Import VRCQuestTools to a Windows project.
  2. Import in any mesh that has vertex colors on it.
  3. Create an object with an avatar descriptor.
  4. Drag the mesh with vertex colors onto the avatar descriptor object such that the mesh becomes the child. The mesh with vertex colors will be stripped of its vertex color information without notice.

Expected behavior On Windows, for the vertex color information to not be stripped without the users consent.

Discussion I maintain a shader (focus.shader.gay) that requires vertex colors to properly work. I have had two users report issues that I was able to track down to being caused by VRCQuestTools' Auto-Remove Vertex Colors feature stripping vertex colors away from a mesh without any notice! I have published instructions instructing users to disable this feature and reimport my shader if they are having issues.

I ask for a reconsideration of the design of the auto-remove vertex color feature. While I understand that removing vertex colors may fix issues on quest avatars, I do not see how doing this silently is a good idea given that this action is destructive. Furthermore, is this feature necessary on Windows?

Perhaps making the auto-remove vertex color feature be something that the user has to explicitly issue would be sufficient. Could be via a button in the 'Convert Avatar for Quest' menu or a new menu that aggregates useful tools for Quest avatars.

Thank you for your time.

kurotu commented 1 year ago

Since most of the target users do not know about vertex colors, it would not be expected that users will spontaneously use the vertex color removal function. At least, this function should be automatically applied by default.

And if an user doesn't have Quest or don't want Quest in testing, they tests converted avatars on Windows. So it's necessary to remove vertex colors also on Windows (especially for me!)

In reconsideration of the feature, I have these ideas. I think B. should be finally applied, how do you feel?

A. Add a confirmation dialog or checkbox to remove vertex colors

B. Duplicate mesh objects to remove vertex colors

valuef commented 1 year ago

I need to preface first by saying that I've never made or tested an avatar for Quest, so I may be entirely misunderstanding the requirements here, but: here are my thoughts:

A. Add a confirmation dialog or checkbox to remove vertex colors

I think a confirmation dialog for vertex color data removal would give users a chance to notice that something has changed about their avatar (in my instance, it'd be that the shader I distribute suddenly disappeared). I think this is an improvement over the current lack of notice!

B. Duplicate mesh objects to remove vertex colors

Duplicated meshes are different from fbx's. This might confuse users when they update fbx.

I have to deal with a similar situation currently and it creates confusion due to the mesh asset being separate from the FBX (User updates model, is confused why the model isn't updated - it's using the saved mesh). Having the user be aware of this isn't that great either as this method generally increases the complexity of the avatar. I've ended up confusing myself just because I forgot that my avatar wasn't using the mesh from the FBX!

Now this may be fixed by having the tool detect that the FBX has been changed and needs to rebake it, but that's a whole different can of beans (looking at checksums? is that peformant? does Unity have something built-in? asset importer callbacks?)

I don't think it's that great of an idea just due to the confusion, but the FBX change detection could alleviate that!

--

Some misc. thoughts and ideas:

Let me know what you think. Thanks!

kurotu commented 1 year ago

Thank you for replying. For the time being, I consider a solution like A. (Auto-Remove Vertex Colors is my first editor extension I made: I was not familiar with Unity than now. Probably I can make better one.)

Perhaps the vertex removal tool doesn't need to immediately act on an avatar? Could it be part of the 'Convert Avatar for Quest' tool and only do its job once the user clicks 'Convert'?

I'll consider this in the solution A. Originally because the colors are restored after restarting Unity, I need to remove colors automatically in order to avoid manually remove colors many times. So now I'm considering to make it opt-out feature like below.

  1. Change 'Remove vertex color' checkbox in the 'Convert Avatar for Quest' window. (Enabled by default)
  2. Press 'Convert'.
  3. The tool marks something like flag to converted avatars.
  4. Automatic removal tool removes vertex colors only from marked avatars.

Could the vertex color changes possibly be applied on upload via an upload callback?

Technically possible. For example, modular-avatar generates temporary meshes in the callback.

However, this approach might not be suitable for vertex colors issue. Quest shaders multiply vertex colors and texture colors, so before removing vertex colors, Quest version of an avatar looks broken (See screenshots of this article). In such situation, users would not press the build button because they believes the avatar is broken.

Now this may be fixed by having the tool detect that the FBX has been changed and needs to rebake it, but that's a whole different can of beans (looking at checksums? is that peformant? does Unity have something built-in? asset importer callbacks?)

Didn't think of that. I'll investigate this approach.

kurotu commented 1 year ago

I released a new version 1.10.0. v1.10.0 has following features.

kurotu commented 8 months ago

Confirmation dialog appears for an avatar whose name contains (Quest)

Now v2.0 removed this feature. So vertex colors are only removed by converter option and VertexColorRemover component.