[Merged by Bors] - GLTF loader: support mipmap filters #1639
Conversation
Ratysz
added
A-Assets
|
Do you think we should emit a warning when ignoring the unsupported filters? Or would that be too noisy? |
|
Well, this is a stop-gap. We will have real mipmap support VerySoon™. I just pulled this commit out of my WIP mipmap work, into its own PR, because I keep seeing people struggle with this error. Also, we aren't technically ignoring them, this code sets them correctly in the texture sampler. It's just that it doesn't make a difference when rendering yet, because the textures don't actually have the mipmaps. No it would not be noisy, I can add a check to print a warning if one of the mipmap filters is used, I just think it's unnecessary. |
|
FYI, the CI failure that happened here is unrelated to my code. It failed at an earlier step (the ubuntu base image packages), before getting to building bevy. |
|
Yeah I agree that doing a best-effort load instead of failing is the better UX. This is an interesting case because Bevy warnings should be "actionable", but in many cases users can't / won't want to change this setting in their input assets. But we also don't want to give users the impression that mipmaps work when they don't. We're stuck between a rock and a hard place 😄 I think you made the right call here, given that this is a gap we will fill in soon. There are a number of other GLTF features that we don't support yet and emitting logs for all of them (by default) would be pretty nasty. Emitting debug logs might be a good middle ground, but I think we should probably just merge this. |
|
bors r+ |
This removes the `GltfError::UnsupportedMinFilter` error. I don't think this error should have existed in the first place, because it prevents users from using assets that bevy could totally render (without mipmap support as of yet). It's much better to load the asset properly and then render it (even if it looks a little ugly), than to refuse to load the asset at all, giving users a confusing error.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
This removes the
GltfError::UnsupportedMinFiltererror.I don't think this error should have existed in the first place, because it prevents users from using assets that bevy could totally render (without mipmap support as of yet).
It's much better to load the asset properly and then render it (even if it looks a little ugly), than to refuse to load the asset at all, giving users a confusing error.