Introduce skid_steer as a new way to load assets - #515
Conversation
7eb271f to
614549c
Compare
614549c to
656444b
Compare
| .image(handle) | ||
| .subresource_range(range)], | ||
| ); | ||
| xf.device.cmd_clear_color_image( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| .material() | ||
| .pbr_metallic_roughness() | ||
| .base_color_factor() | ||
| .map(|c| 255) // TODO: Will likely want a crate for color conversion |
There was a problem hiding this comment.
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.
656444b to
2ac99ab
Compare
2ac99ab to
359c1aa
Compare
Ralith
left a comment
Ralith
left a comment
There was a problem hiding this comment.
Review is just getting started, but it may be a few days before I make more progress.
| } | ||
|
|
||
| fn get_all_events(&mut self) -> Vec<Event> { | ||
| let events = self.events.lock().unwrap().clone(); |
There was a problem hiding this comment.
nit: &mut self means we have exclusive access, so there's no need to mess around with locking. Prefer get_mut.
| /// Drains all events that were returned by the most recent call to get_all_events | ||
| fn drain_queried_events(&mut self) { | ||
| self.events | ||
| .lock() |
| } | ||
| } | ||
|
|
||
| fn test_eventual_success(mut f: impl FnMut() -> Result<(), anyhow::Error>) { |
There was a problem hiding this comment.
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.
| impl AssetLoadContext { | ||
| /// # Safety | ||
| /// - [`Work::cmd`] must not be used outside the lifetime of the returned [`Work`] | ||
| /// - Any Vulkan resources this work uses must not be destroyed before the [`Work`] is dropped |
There was a problem hiding this comment.
Surely they must outlive the execution of the work?
|
|
||
| /// 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. |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| #[test] | ||
| #[cfg_attr(not(feature = "run_vulkan_tests"), ignore)] |
There was a problem hiding this comment.
Let's just install lavapipe and maintain full test coverage in CI.
| .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 |
There was a problem hiding this comment.
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.
| ) { | ||
| loop { | ||
| // Systems increment the `queue_unpark_semaphore` value when they want to guarantee | ||
| // that we don't park unless certain things are done. `ParallelQueueWaiter` |
| 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. |
There was a problem hiding this comment.
Why is this needed? Submitting work should unpark the queue automatically.
| queue_unpark_semaphore: vk::Semaphore, | ||
| shutdown_token: CancellationToken, | ||
| queue_watch_sender: &tokio::sync::watch::Sender<u64>, | ||
| staging: &GrowableRing, |
There was a problem hiding this comment.
Consider aggregating this stuff into a struct with a fn run(self).
2 participants
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:
AssetLoadertype is added to drive asset loading, acting as the glue between skid_steer and Hypermine. This completely replaces the oldloadermodule.lahar_deprecatedmodule have been replaced with a vendoredgrowable_ringimplementation from @Ralith, and aParallelQueuefrom the current version of lahar. This allows us to finally remove thelahar_deprecatedmodule, a task that I had hoped to do years ago.Loader, have been migrated to the newAssetLoader.ShaderDatastruct to pass around global data used for rendering.