extensions: make naming and layout consistent across all extensions #494

filnet · 2021-11-12T14:17:26Z

breaking change:

some extensions were exposing instance() instead of device()

includes:

renaming function pointer member to fns
moving name(), fp(), device()/instance() functions at end of file
adding missing device()/instance() functions
~~using result() instead of into()~~

ash/src/extensions/ext/extended_dynamic_state.rs

ash/src/extensions/khr/push_descriptor.rs

MarijnS95 · 2021-11-16T09:47:25Z

I guess we want to get this in after #495?

filnet · 2021-11-16T14:24:57Z

I guess we want to get this in after #495?

The two PRs are independent AFAIK.

MarijnS95 · 2021-11-16T14:45:48Z

This PR also converts some of the .into() calls to .result(). Those should disappear as soon as #495 is merged, hence the suggestion to merge that one in first (and reduce the size of this diff a little).

filnet · 2021-11-16T16:27:13Z

This PR also converts some of the .into() calls to .result(). Those should disappear as soon as #495 is merged, hence the suggestion to merge that one in first (and reduce the size of this diff a little).

Ok. Makes sense.

ash/src/extensions/ext/debug_marker.rs

ash/src/extensions/ext/debug_utils.rs

ash/src/extensions/khr/pipeline_executable_properties.rs

ash/src/extensions/khr/timeline_semaphore.rs

MarijnS95

Not seeing any red flags with this PR, looks good overall. Thanks for the hard work!

filnet · 2021-11-19T12:17:32Z

Not seeing any red flags with this PR, looks good overall. Thanks for the hard work!

You're welcome.

Just pushed some commit log rewording.

Changelog.md

filnet · 2021-11-19T12:27:02Z

There are two "extensions" I left untouched:

ext::PhysicalDeviceDrm
experimental::amd

breaking change: - some extensions were exposing `instance()` instead of `device()` includes: - renaming function pointer member to `fns` - moving `name()`, `fp(`), `device()`/`instance()` functions at end of file - adding missing `device()`/`instance()` functions see ash-rs#493

…name()`

Changelog.md

MarijnS95 · 2021-11-26T11:44:49Z

Changelog.md

+### Changed
+
+- Device extension `ext::ExtendedDynamicState` now exposes `fn device()` instead of `fn instance()` (#494)
+- Device extension `khr::PushDescriptor` now exposes `fn device()` instead of `fn instance()` (#494)
+- Device extension `khr::PipelineExecutableProperties` now exposes `fn device()` instead of `fn instance()` (#499)
+- Changed `khr::PipelineExecutableProperties::new()` to take `instance` and `device` as arguments (#499)
+- Device extension `khr::TimelineSemaphore` now exposes `fn device()` instead of `fn instance()` (#499)
+- Changed `khr::TimelineSemaphore::new()` to take `instance` and `device` as arguments (#499)
+
+### Removed
+
+- Removed `device` argument from `ext::DebugMarkers::debug_marker_set_object_name` function (#494)
+- Removed `From<vk::Result>` trait for `VkResult` (#495)
+- Removed `instance` argument from `ext::DebugUtils::submit_debug_utils_message` function (#499)
+- Removed `device` argument from `khr::PipelineExecutableProperties` functions (#499)
+- Removed `device` argument from `khr::TimelineSemaphore` functions (#499)


Thanks, also for taking care of #499. Just not sure if we should mention some single PRs this often in the changelog.

For example, the ### Removed section is afaik for removed API - changing the arguments to remove ie. device that's already available in the struct is already documented under ### Changed, no need to repeat that again.

Furthermore, we can probably make lists, ie turn this:

- Device extension `ext::ExtendedDynamicState` now exposes `fn device()` instead of `fn instance()` (#494) - Device extension `khr::PushDescriptor` now exposes `fn device()` instead of `fn instance()` (#494) - Device extension `khr::PipelineExecutableProperties` now exposes `fn device()` instead of `fn instance()` (#499)

into:

- Device extensions `ext::ExtendedDynamicState`, `khr::PushDescriptor` and `khr::PipelineExecutableProperties` now expose `fn device()` instead of `fn instance()` (#494, #499)

I think that's much more more concise (more readable).

@Ralith What are your thoughts on this, and this PR in general? Seems to be pretty much ready to go in besides some changelog bikeshedding, unless you have some burning comments?

Would like to get this in and do a release soon, it has been way too long.

Your proposed tweaks to the changelog SGTM, and overall this looks like great cleanup, though I haven't reviewed every single change in detail.

The changes are pretty boring. It's mostly code gardening.
Could help if we want to generate that code in the future though.

PS: Redacting the Changelog is actually harder than it was doing the code changes.

@filnet Yes, curating the changelog (usually when preparing for a release) is pretty tough. We usually end up going for just the commit titles since the last release, but that's usually not fully representative nor are all the titles good enough for direct use :)

ash/src/extensions/ext/extended_dynamic_state.rs

Ralith · 2021-11-26T19:22:53Z

Changelog.md

+### Changed
+
+- Device extension `ext::ExtendedDynamicState` now exposes `fn device()` instead of `fn instance()` (#494)
+- Device extension `khr::PushDescriptor` now exposes `fn device()` instead of `fn instance()` (#494)
+- Device extension `khr::PipelineExecutableProperties` now exposes `fn device()` instead of `fn instance()` (#499)
+- Changed `khr::PipelineExecutableProperties::new()` to take `instance` and `device` as arguments (#499)
+- Device extension `khr::TimelineSemaphore` now exposes `fn device()` instead of `fn instance()` (#499)
+- Changed `khr::TimelineSemaphore::new()` to take `instance` and `device` as arguments (#499)
+
+### Removed
+
+- Removed `device` argument from `ext::DebugMarkers::debug_marker_set_object_name` function (#494)
+- Removed `From<vk::Result>` trait for `VkResult` (#495)
+- Removed `instance` argument from `ext::DebugUtils::submit_debug_utils_message` function (#499)
+- Removed `device` argument from `khr::PipelineExecutableProperties` functions (#499)
+- Removed `device` argument from `khr::TimelineSemaphore` functions (#499)


Your proposed tweaks to the changelog SGTM, and overall this looks like great cleanup, though I haven't reviewed every single change in detail.

ash/src/extensions/ext/full_screen_exclusive.rs

Changelog.md

Ralith

Approving wrt. the issues I've previously specifically highlighted.

MarijnS95

Changelog seems fine besides this - will re-re-re-review the rest of the code later on :)

Changelog.md

… functions

…etter

MarijnS95

This all looks good now. Thanks for your perseverance in dealing with all the review comments and repeated large changes!

filnet commented Nov 12, 2021

View reviewed changes

ash/src/extensions/ext/extended_dynamic_state.rs Show resolved Hide resolved

filnet commented Nov 12, 2021

View reviewed changes

ash/src/extensions/khr/push_descriptor.rs Show resolved Hide resolved

filnet force-pushed the extension_cleanup branch 5 times, most recently from cda2c50 to bb29817 Compare November 15, 2021 22:31

filnet force-pushed the extension_cleanup branch 2 times, most recently from cb41ef9 to 6eef4c7 Compare November 16, 2021 20:58