(WIP) Refactor asset loading by patowen · Pull Request #22 · patowen/hypermine

patowen · 2026-08-02T22:56:46Z

Very rough draft of new form of asset loading.

When testing this, I edited lahar::ParallelQueue to specify .flags(vk::SemaphoreWaitFlags::ANY) in its park method, as that is surprisingly not the default.

Recommendations for reviewing

The most important changes are in png_array.rs (used as an example for the new system) and asset_loader.rs (the new asset loader logic)
Hiding whitespace will help highlight the changes in png_array.rs. I elected not to do some of the refactors I originally planned yet, such as copying image data into Rust managed memory before copying into a Vulkan buffer, because I would be able to make the diff smaller that way, but only if whitespace is hidden, as the indentation level has changed.
The "offset_allocator" module can be ignored. That is a dependency I vendored into the branch for later, but it is not in use so far.

…ation

Ralith · 2026-08-03T01:15:30Z

I added a timeout of 1 second to lahar::ParallelQueue so that park won't block indefinitely

This shouldn't be necessary; waking it up on demand is what the passed-in semaphore is for.

…Queue

Ralith · 2026-08-03T01:17:26Z


    pub fn transforms_mut(&mut self) -> &mut [na::Matrix4<f32>] {
-        &mut self.transforms
+        unsafe { self.transforms.as_mut() }


nit: best practice (which we're absolutely not following consistently in existing code) is to always include a comment on unsafe blocks explaining why the operations within are sound, with the possible exclusion of blindingly obvious cases. In this example, it would explain why there's no aliasing hazard.

Thanks for the heads up. I'll look into this during my final polishing pass.

EDIT: Or now, now that I realize that what I'm looking for is concurrent writes by the GPU

EDIT2: I'll look into this later. It's still a bit tricky for me to fully understand its safety requirements.

Ralith · 2026-08-03T01:22:12Z

-    free_nodes: FreeNodeStack<NI>,
-}
-
-struct NodeMap<NI: NodeIndex>(Vec<Node<NI>>);


What was wrong with the stack? A slab is technically slightly more book-keeping.

Hmm, maybe. I thought it was the ideal use case of a Slab, since it behaves as a key/value store for node indexes. I found the setup of the FreeNodeStack to be not ideal, especially since max_allocs is used to populate it.

I'll think more about this.

I didn't review this change closely enough to have an informed opinion about how it interacts with the other logic, since you recommended focusing elsewhere. If naturally providing a stable key simplifies code elsewhere compared to the stack, that would be a very good reason for the change.

I agree with not looking into this too closely. To answer your question, though, the IDs were always stable. It just used to be implemented manually using two arrays. One of them was the one referenced with stable IDs, and the other acted as a stack and contained the full list of free IDs you could draw from. Its interface basically already matched the interface of a slab.

Because of that, when I was thinking of how to abstract that logic, the name of the data structure I wanted to use was "slab", and since we already depended on the slab crate, it seemed silly not to use that.

However, looking at the implementation of Slab, it seems like there might be a fair amount of memory overhead, as there's an enum with a usize as one of its options, which I think means that each slot takes at least 128 bits.

One of them was the one referenced with stable IDs, and the other acted as a stack and contained the full list of free IDs you could draw from. Its interface basically already matched the interface of a slab.

Ah, that sounds like a fair reason to simplify with a real slab then! If upstream has a good reason not to do that, they should at least document it.

which I think means that each slot takes at least 128 bits.

This is unlikely to matter.

Ralith · 2026-08-03T01:42:18Z

 }
+
+impl skid_steer::Source for PngArray {
+    type Output = DedicatedImage; // TODO: We may want a dedicated struct here.


Why would we need a dedicated struct?

I left that comment there thinking I would be tripped up by having the asset loader return an image without any additional metadata, but I haven't reached that point yet.

I could imagine situations where the asset would want to retain additional data, such as the names of the materials it loaded. However, given Rust's type safety, I think that if that need arises, the refactor would be easy. Since we haven't needed a dedicated struct yet, I'll remove the comment.

Ralith · 2026-08-03T04:00:47Z

+        align: usize,
+        free_at: u64,
+    ) -> Allocation<'_> {
+        // TODO: Instead of blocking in this haphazard way, try growing the timeline ring instead if it's too small.


This may be necessary to avoid deadlocks in control flow paths that cross multiple allocations, which may occur in very difficult to reproduce ways with concurrent allocation traffic.

Indeed, I do agree it's worth redesigning that part.

Ralith · 2026-08-03T04:05:07Z


 impl PngArray {
    async fn load_inner(self, context: &skid_steer::Context<'_>) -> anyhow::Result<DedicatedImage> {
+        println!("Started loading png array");


I recommend getting comfortable with using tracing logs even (perhaps especially) for debugging, as it's easy to attach useful context to them via dynamic scope, and to include diagnostics that are normally inactive but don't need to be reinvented every time you're investigating an issue.

That's a fair point. It would be nice to not have to delete all the logs before submitting a PR, especially when they've proved to be useful in the past. I am pretty comfortable with using tracing logs, but I guess not comfortable enough to reach to them as a default.

It may help to adopt the test-log crate, and ensure your development environment sets RUST_LOG to a useful configuration by default.

…ing and mesh generation

patowen added 24 commits April 27, 2026 19:03

Upgrade Lahar

c55ca3e

Vendor the offset_allocator dependency

eab7739

Use a slab for allocation instead of an ad-hoc stack

d295d3a

Improve documentation of the offset allocator

29d9b70

Add license of offset-allocator

e07ace8

Rename max allocations to max nodes for improved accuracy of document…

6379caf

…ation

Continue work on implementing updated png_array

e190b0b

Continue adding to png_array

b883c60

Finish skeleton png_array implementation

05a4176

Start throwing together something for a driver for asset loading

481e75f

Try to make AssetLoader more usable

8130ab7

Further flesh out asset_loader

460e030

Park the parallel queue

222deba

Set up atomic to track the timeline queue's progress

078453a

Integrate parallel-queue-waiting into AssetLoader's logic

040aac4

Fold loop better in wait_for_semaphore

5f195ca

Read the semaphore directly in ParallelQueueWaiter

796e1ff

Attempt to make asset_loader blocking

ec41bda

Introduce AsyncTimelineRing

3ea7979

Property drive the AsyncTimelineRing

11b27ef

Try actually testing AssetLoader (not yet working)

56f7c38

Remove WorkQueue, as it has been unused since pr Ralith#451

083ced3

Add Config to the context to resolve immediate crash

f2cfe88

Create something working for png_array

b1cc5b6

patowen changed the title ~~Refactor vulkan wip~~ (WIP) Refactor asset loading Aug 2, 2026

Remove old png_array implementation

ba0abb1

Increase scope of the semaphore's capabilities to unpark the Parallel…

af7e50e

…Queue

Ralith reviewed Aug 3, 2026

View reviewed changes

Make small changes to begin addressing comments

f062ca2

patowen added 3 commits August 5, 2026 20:01

Add parallel queue to tests

8adf22e

Simplify asset loader and find new bugs

ce80444

Simplify arguments of run_task_executor

223c79e

patowen force-pushed the refactor-vulkan-wip branch from 87e2e61 to 223c79e Compare August 7, 2026 03:18

patowen added 26 commits August 6, 2026 23:54

Use a shutdown token insetad of an mpsc

cccd16b

Only drop handles after queue has been drained

2deccbc

Resolve additional soundness issues

0ff7f27

Keep sending parallel queue status for drained queue

5e2a372

Remove one layer of abstraction to fix cancellation logic

07d4de6

cargo fmt

1d0b874

Rename some things

4c847d5

Improve naming, logging, and documentation

2427a7b

Simplify logic based on parallel_queue::Handle::drain

1a48817

Fix unsound handling of queue unparking

40b3b75

Merge branch 'master' into refactor-vulkan-wip

e138ec3

Remove cancel-safety requirements from AssetLoader

88ea57d

Remove cancel-safety attempt from png_array

f12acc0

Address some TODOs

1df97f3

Fully separate png_array image I/O with Vulkan wrangling

76b0bb9

Split shader_data into separate struct (and start splitting GLTF read…

daa9108

…ing and mesh generation

Move asset_loader to graphics module

8b0b39e

Add constructor for MeshGeometry

b49da25

Add constructor for MeshMaterial

170c05f

Add constructor for Mesh

ab18f39

Switch character asset to skid_steer version

34bc245

Fix deadlock

89f14c2

Add warning for potential deadlock

9188522

Remove old Loader

39c32d6

Delete lahar_deprecated module

cd5438a

Use alternative Loader implementation for draining

6dd2781

Conversation

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

Uh oh!

Recommendations for reviewing

Uh oh!

Ralith commented Aug 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 3, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patowen Aug 3, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

patowen commented Aug 2, 2026 •

edited

Loading

patowen Aug 3, 2026 •

edited

Loading

patowen Aug 3, 2026 •

edited

Loading