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

[Merged by Bors] - load zeroed UVs as fallback in gltf loader #1803

Closed
wants to merge 3 commits into from

Conversation

jakobhellermann
Copy link
Contributor

fixes a lot of gltf loading failures (see #1802)

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 2, 2021
@mockersf
Copy link
Member

mockersf commented Apr 3, 2021

a message should be logged to notify the user of the change, and let them know if they want to fix it in their modelling tool

@jakobhellermann
Copy link
Contributor Author

a message should be logged to notify the user of the change, and let them know if they want to fix it in their modelling tool

I added a log.

} else {
let len = mesh.count_vertices();
let uvs = vec![[0.0, 0.0]; len];
bevy_log::warn!("missing `TEXCOORD_0` vertex attribute, loading zeroed out UVs");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warnings (and to a lesser extent, info logs) in Bevy must be actionable/fixable. But there will be cases where users won't be able to modify models (ex: they bought them on an asset store). I think we should consider "uv-less" models a "happy path" case.

Can we make this a debug log instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@cart
Copy link
Member

cart commented Apr 13, 2021

I'm going to hold off on merging this until we have this conversation: #1808 (comment)

We should sort out a short term and long term plan for "missing vertex attributes". Long term I'd like to support loading "minimal" meshes without generating vertex attributes when they aren't needed, or warning/logging unnecessarily.

@cart
Copy link
Member

cart commented Apr 13, 2021

Short term these changes are super valuable / make gltf loading more usable, so I'm willing to make concessions. I just want to be thoughtful about our first steps.

@jakobhellermann
Copy link
Contributor Author

The two possible solutions are

  1. add zeroed buffer for UVs
  2. handle missing UVs in the shader (like Allow for usage of GLTF primitives that have no texcoords #1010)

In #1010 it seemed like you preferred zeroed buffers as the solution.

Am I missing any alternatives?

@cart
Copy link
Member

cart commented May 6, 2021

(sorry for the delay)

I think generating UVs is probably the right call, but as I called out here, I think long term this should probably be a "mesh transform" users opt in to rather than something that happens implicitly. I expect there to be a class of meshes that don't need certain vertex attributes. Rather than bloat them with those attributes, I'd rather detect cases of "shader expected attributes that the mesh doesn't have", then enable the user to opt-in to generating things like "zeroed out uvs".

Currently we don't support "asset config", so theres no good way to opt in to these transformations. For now, I'm down to merge this.

@cart
Copy link
Member

cart commented May 6, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 6, 2021
fixes a lot of gltf loading failures (see #1802)
@bors bors bot changed the title load zeroed UVs as fallback in gltf loader [Merged by Bors] - load zeroed UVs as fallback in gltf loader May 6, 2021
@bors bors bot closed this May 6, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants