Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consider providing a Device::set_debug_utils_object_name shortcut on individual objects #2242

Closed
HadrienG2 opened this issue Jun 29, 2023 · 3 comments · Fixed by #2275
Closed

Comments

@HadrienG2
Copy link
Contributor

HadrienG2 commented Jun 29, 2023

This could be done by adding a set_debug_utils_name method to the VulkanObject of DeviceOwned trait, with an appropriate where bound so that it is only available where the other trait is present. From my perspective, it would make a little more sense to put it on the VulkanObject trait, since it's a Vulkan thing.

The rationale for this proposal is that DeviceOwned objects, by definition, know which device they belong to. And from this perspective, needing to get my hand to a &Device just to set their debug utils name feels like unnecessary ceremony.

Further, use of Vulkan requires creating a lots of objects, which in an ideal world would all be annotated with names in debugging/profiling mode so that tools provide more explicit output. Therefore, the ergonomics benefits of such a shortcut would quickly add up.

@Rua
Copy link
Contributor

Rua commented Jun 29, 2023

Good idea! Do you think we still need the method on Device after this is implemented?

@HadrienG2 HadrienG2 changed the title Condier providing a Device::set_debug_utils_object_name shortcut on individual objects Consider providing a Device::set_debug_utils_object_name shortcut on individual objects Jun 29, 2023
@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Jun 29, 2023

If we remove the method on Device, then the functionality will only be available via a trait, which may make it harder to discover by users.

Also, while playing with this in parallel, I realized that ergonomics of debug utils names could be further improved by making it possible to call set_debug_utils_name directly on a Vulkano object that maps to some underlying Vulkan object, like StorageImage or SubBuffer.

This could be an argument for having some sort of BorrowVulkanObject trait, that debug utils name setup would then accept as a bound instead of VulkanObject.

@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Jun 29, 2023

...also, I think I found a little use after free problem in the implementation :(

    pub fn set_debug_utils_object_name<T: VulkanObject + DeviceOwned>(
        &self,
        object: &T,
        object_name: Option<&str>,
    ) -> Result<(), OomError> {
        assert!(object.device().handle() == self.handle());

        // Building a CString, so far, so good...
        let object_name_vk = object_name.map(|object_name| CString::new(object_name).unwrap());
        let info = ash::vk::DebugUtilsObjectNameInfoEXT {
            object_type: T::Handle::TYPE,
            object_handle: object.handle().as_raw(),
            // ...but this map_or moves the CString, which will be dropped once this statement is done evaluating...
            p_object_name: object_name_vk.map_or(ptr::null(), |object_name| object_name.as_ptr()),
            ..Default::default()
        };

        // ...and thus the pointer will be dangling by the time we call Vulkan.
        unsafe {
            let fns = self.instance().fns();
            (fns.ext_debug_utils.set_debug_utils_object_name_ext)(self.handle, &info)
                .result()
                .map_err(RuntimeError::from)?;
        }

        Ok(())
    }

I think this can be fixed by using as_ref() to borrow the CString, instead of moving it (and later dropping it), as follows:

            p_object_name: object_name_vk.as_ref().map_or(ptr::null(), |object_name| object_name.as_ptr()),

Rua added a commit to Rua/vulkano that referenced this issue Jul 29, 2023
marc0246 added a commit that referenced this issue Aug 5, 2023
* Improve debug messenger, remove second `Instance` constructor

* #2242

* Doctest fix

* Take Arc out

* DeviceOwnedVulkanObject

* Update vulkano/src/device/mod.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Update vulkano/src/instance/debug.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Fixes

---------

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
hakolao pushed a commit to hakolao/vulkano that referenced this issue Feb 20, 2024
…o-rs#2275)

* Improve debug messenger, remove second `Instance` constructor

* vulkano-rs#2242

* Doctest fix

* Take Arc out

* DeviceOwnedVulkanObject

* Update vulkano/src/device/mod.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Update vulkano/src/instance/debug.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Fixes

---------

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants