Introduce skid_steer as a new way to load assets by patowen · Pull Request #515 · Ralith/hypermine

patowen · 2026-08-17T02:29:46Z

This is a relatively significant refactor of Hypermine, switching up asset loading to use a new library called "skid_steer".

It introduces the following changes:

A custom AssetLoader type is added to drive asset loading, acting as the glue between skid_steer and Hypermine. This completely replaces the old loader module.
All uses of the staging buffer and transfer module from the temporary lahar_deprecated module have been replaced with a vendored growable_ring implementation from @Ralith, and a ParallelQueue from the current version of lahar. This allows us to finally remove the lahar_deprecated module, a task that I had hoped to do years ago.
PNG loading and GLTF loading, the only two processes that depended on the old Loader, have been migrated to the new AssetLoader.
File-processing (reading PNGs and GLBs) and Vulkan-wrangling (making Vulkan calls) have been fully separated, with all data being read into memory in a convenient form before being passed into Vulkan data structures.
To support some of the above changes, some data has been moved around, including the addition of a new ShaderData struct to pass around global data used for rendering.

patowen · 2026-08-22T02:51:33Z

-                        .image(handle)
-                        .subresource_range(range)],
-                );
-                xf.device.cmd_clear_color_image(


When separating out gtlf-reading logic with Vulkan wrangling, I had decided to remove the functionality that uses cmd_clear_color_image to fill the 1x1 image with a specified color, in favor of just specifying the color directly.

However, this requires a conversion from f32 color to SRGB, which you might have opinions on how to do. One option could be to import something like https://crates.io/crates/palette (although you might have a preferred crate for this). Given that we might want to do more in the future, I don't think we want to hardcode the gamma correction formula.

I haven't gotten to the context of this question yet, but the color crate is an alternative to consider for color space conversions; it's maintained by some extremely reputable 2D graphics folks. I don't think we need an external crate for gamma conversion but it's fine to use one.

I might consider the color crate. Although implementing it myself (or copying color's implementation would be pretty simple, it's complicated by the fact that the gamma conversion function is piecewise, starting linear and ending with a power function, and I also have to convert from a float to a u8, which has its own rounding standards for color (that I don't remember off the top of my head). My understanding is that when something is messy due to existing standards, it's a good sign that it would be best to reuse an existing implementation.

patowen · 2026-08-22T02:54:11Z

+                    .material()
+                    .pbr_metallic_roughness()
+                    .base_color_factor()
+                    .map(|c| 255) // TODO: Will likely want a crate for color conversion


See https://github.com/Ralith/hypermine/pull/515/changes/BASE..0908719a3be8e59af41e30e075cbe97f0001cf17#r3834892686.

I deliberately kept the lint failing due to this line so that I wouldn't forget to address this.

EDIT: Actually, now that there's an unresolved PR comment, this shouldn't be forgotten anyway. I'll fix the lint.

Ralith

Review is just getting started, but it may be a few days before I make more progress.

Ralith · 2026-08-26T03:40:26Z

+        }
+    }
+
+    fn test_eventual_success(mut f: impl FnMut() -> Result<(), anyhow::Error>) {


This is complex and may still be fragile; there's no hard guarantee on a desktop OS that any particular thread will be scheduled in any particular time interval. Prefer to actually wait for the event we're interested in. For example, you could use a std::sync::Condvar to implement a helper that blocks until an event is delivered, and call that in a loop.

I don't see how that would make it more robust. Either the condition variable is triggered, and the test passes, or the condition variable is not, and the test hangs. In the latter, I would prefer the test to fail with a timeout error. The three-second timeout I added should be orders of magnitude longer than it would actually take.

I feel like if the asset loader hangs for 3 seconds for no discernible reason, that would be a bug in the asset loader, not a bug in the test.

While the function feels complex, at it's core, it's quite simple: Keep trying until it succeeds, or until it's time to give up.

If we want to be more robust than that, we'd need to dig into the implementation details of skid_steer and detect when it's no longer going to make progress.

In the latter, I would prefer the test to fail with a timeout

Condvar with wait_timeout, then?

That might work. I guess the idea is to specialize this function to wait for events to show up instead of waiting an arbitrary amount of time with exponential backoff.

My next force-push will include an attempt to resolve this.

EDIT: Done

Ralith · 2026-08-26T03:52:31Z

+
+/// Timeline sempahores must be signaled in a strictly increasing sequence, so having multiple threads manage
+/// the semaphore that unparks the timeline queue is error-prone. The purpose of this thread is to centralize
+/// management of this semaphore.


Do we actually need to trigger this from multiple threads? If so, would it be simpler to use a Mutex<u64> that we hold across the signal operation?

Since sending work for submission does not on its own unpark the parallel queue, we need to trigger this every time we send work for submission. This probably would be simpler with a mutex. It can be tricky at times to weigh options between different synchronization primitives.

I do like this version because it only signals the semaphore even if everyone tries to signal it at the same time, although it's unclear whether that's worth running an extra thread.

Signaling should not be an expensive operation.

My next force-push will include an attempt to resolve this.

EDIT: Done

Ralith · 2026-08-26T03:57:34Z

+        .expect("runtime using this context should already be dropped");
+
+    // Fail-safe to ensure that the parallel queue is driven at least once after all work has been submitted before
+    // we drain the handle


Why would that be needed? If we don't intend for it to be needed, then this will prevent us from detecting bugs with tests.

So, thinking about it, I'm not sure whether we need this fail-safe.

On one hand, we need it because if a skid_steer::Source submits work without waiting on it, trying to drain the handle without driving the queue would result in a deadlock. After all, in the standard flow, waiting on the queue is what generally triggers the queue to be unparked (due to the implementation of wait_for_completion).

On the other hand, no asset sources should do that, since it would be unsound in almost all situations. I generally prefer crashes or validation errors over deadlocks though, since they tend to be easier to pin down.

Overall, I'm still in favor of keeping it. I think that it's more likely to ease debugging than make it more difficult (due to passing tests).

How does this behavior help trigger a crash or validation error?

Would it be more useful to detect that class of bug by panicking if a load task finishes without waiting for work it submitted?

I'm not convinced it is useful enough to be worth the extra scaffolding. The solution currently in place seems simplest, as it's essentially a way to ensure that the call to Handle::drain (which comes right after it) is preceded by a guarantee that the queue will be driven.

To answer your question on how it would help trigger a crash or validation error, it would ensure that the work sent for submission is actually submitted. If said work depends on resources that were already destroyed, that should trigger the validation error.

An alternative possibility is to let the deadlock happen, but in that case, I'd likely want to wrap it in a timer that at least logs a helpful warning if it runs out.

Thinking about this further, I feel like the current implementation is the one I prefer, as it follows the principle of least surprise, as we shouldn't need to expect draining the handle to deadlock. I consider unparking the semaphore to be a hard prerequisite to calling Handle::drain. After all, if the gap mentioned in #515 (comment) were closed, this would have happened automatically, and we wouldn't even be having this discussion (even though the soundness risk would still be present).

I don't think there's a clean, reliable, non-over-engineered way to check whether the load task waited for the work it submitted, since load functions are not currently required to report the necessary information to trigger this panic.

Ralith · 2026-08-26T04:14:05Z

-    pub fn queue_family(&self) -> u32 {
-        self.gfx.queue_family
+    pub async fn wait_for_completion(&self, semaphore_value: u64) {
+        // To actually get the work to start, we need to unpark the queue. We do it here to avoid getting stuck awaiting something we never kicked off.


Why is this needed? Submitting work should unpark the queue automatically.

Submitting work does not unpark the queue automatically. The only thing calling Work::end does is send it to the mpsc. The receiver of this mpsc only activates when ParallelQueue::drive is called, and that is what triggers the queue_submit function.

I should probably double-check the comments to make sure I say "sent for submission" instead of "submitted" in the appropriate places, since they are subtly different.

Whoops, right you are. We could close that gap, but I guess it's nice to be able to batch stuff up.

Ralith · 2026-08-26T04:15:05Z

+    queue_unpark_semaphore: vk::Semaphore,
+    shutdown_token: CancellationToken,
+    queue_watch_sender: &tokio::sync::watch::Sender<u64>,
+    staging: &GrowableRing,


Consider aggregating this stuff into a struct with a fn run(self).

My next force-push will include an attempt to resolve this.

EDIT: Done

patowen force-pushed the introduce-skid-steer branch 3 times, most recently from 614549c to 656444b Compare August 22, 2026 02:41

patowen commented Aug 22, 2026

View reviewed changes

patowen requested a review from Ralith August 22, 2026 03:06

patowen mentioned this pull request Aug 23, 2026

(WIP) Refactor asset loading patowen/hypermine#22

Closed

patowen force-pushed the introduce-skid-steer branch 2 times, most recently from 2ac99ab to 359c1aa Compare August 25, 2026 02:21

patowen marked this pull request as ready for review August 26, 2026 02:39

Ralith requested changes Aug 26, 2026

View reviewed changes

patowen added 6 commits August 28, 2026 00:18

Create a new AssetLoader to eventually replace Loader

28bd51e

Add Vulkan functionality to new asset_loader

0a31a50

Use skid_steer to load png_array

225c303

Refactor: Gather global data sent to shaders into a single struct

2cd9d06

Use skid_steer to load meshes

5086dd3

Delete code related to old Loader

cd2bff8

patowen force-pushed the introduce-skid-steer branch from 359c1aa to cd2bff8 Compare August 28, 2026 04:22

Conversation

patowen commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patowen Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ralith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

patowen commented Aug 17, 2026 •

edited

Loading

patowen Aug 22, 2026 •

edited

Loading

patowen Aug 22, 2026 •

edited

Loading

patowen Aug 28, 2026 •

edited

Loading

patowen Aug 27, 2026 •

edited

Loading

patowen Aug 28, 2026 •

edited

Loading

patowen Aug 27, 2026 •

edited

Loading

patowen Aug 28, 2026 •

edited

Loading

patowen Aug 28, 2026 •

edited

Loading