vello_hybrid: Add Resources::new_with_config#1750
Conversation
LaurenzV
left a comment
LaurenzV
left a comment
There was a problem hiding this comment.
Yeah, we definitely need to fix this, for now let's just go with this I guess!
| Self::new_with_config(AtlasConfig::default()) | ||
| } | ||
|
|
||
| /// Create a new set of renderer resources with a custom image/glyph atlas configuration. |
There was a problem hiding this comment.
Let's please add the TODO here that we need to fix this, so we don't forget about it.
Also, we later on normalize the config in the renderer against the device limits, so it's possible they still don't match, but yes let's leave that for a follow-up.
| @@ -0,0 +1,92 @@ | |||
| // Copyright 2026 the Vello Authors | |||
There was a problem hiding this comment.
I'm not sure this test will run in CI if we put it here. Can we not put it into vello_sparse_tests, still as a custom module?
Also, let's add a TODO that upload_image doesn't unwrap in the first place. Since this is what should fail in the first place, can we not reduce the test to just check that upload_image doesn't work? Like, there is no point in having any of the stuff after this for that specific test (i.e. drawing it into the scene), no?
There was a problem hiding this comment.
Moved this into vello_sparse_tests as a new webgl-only image_atlas module. Reduced it to just asserting upload_image fails with TextureTooLarge. Also added a TODO at the allocate(...).unwrap(). (FWIW the old native_webgl/tests location did run in CI via the native-webgl wasm-pack job, but agreed it's cleaner in the dedicated test crate.)
| Self::new_with_config(AtlasConfig::default()) | ||
| } | ||
|
|
||
| /// Create a new set of renderer resources with a custom image/glyph atlas configuration. |
| @@ -0,0 +1,92 @@ | |||
| // Copyright 2026 the Vello Authors | |||
There was a problem hiding this comment.
Moved this into vello_sparse_tests as a new webgl-only image_atlas module. Reduced it to just asserting upload_image fails with TextureTooLarge. Also added a TODO at the allocate(...).unwrap(). (FWIW the old native_webgl/tests location did run in CI via the native-webgl wasm-pack job, but agreed it's cleaner in the dedicated test crate.)
2 participants
Summary
The image atlas is currently configured in two independent places: the renderer's settings (which size the GPU atlas texture) and the
ImageCacheowned byResources(which is what actually allocates images and reports errors). When these two disagree, theAtlasError::TextureTooLargeerror is never surfaced — the too-large check runs against the wrong config, so uploading an image that exceeds the intended atlas size silently proceeds instead of failing.This PR adds a public
Resources::new_with_config(AtlasConfig)so callers can configure the atlas that Resources owns, letting both configs be kept in sync. It also adds a browser WebGL test that sets up a small (10×10) atlas and asserts that uploading a larger image fails withTextureTooLarge.This is a band-aid: it makes the error reachable, but the caller is still responsible for passing matching configs in two places. A proper fix should unify this, likely a single source of truth via a unified resource-management approach, and is left for follow-up.
Created with assistance from Claude Opus 4.8 High.