Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Incorrect union types #40

Closed
serenity4 opened this issue Mar 7, 2021 · 17 comments
Closed

Incorrect union types #40

serenity4 opened this issue Mar 7, 2021 · 17 comments

Comments

@serenity4
Copy link
Member

The unions are currently not generated correctly. I believe this will be fixed by JuliaInterop/Clang.jl#278 (do you confirm @Gnimuc?), though I thought it would be nice to track it somewhere.

For example, the XML specification for the VkClearColorValue type lists:

<type category="union" name="VkClearColorValue" comment="// Union allowing specification of floating point, integer, or unsigned integer color data. Actual value selected is based on image/attachment being cleared.">
    <member><type>float</type>                  <name>float32</name>[4]</member>
    <member><type>int32_t</type>                <name>int32</name>[4]</member>
    <member><type>uint32_t</type>               <name>uint32</name>[4]</member>
</type>

and, in Julia, we only have the float part of the union correct:

struct VkClearColorValue
    float32::NTuple{4, Cfloat}
end
@Gnimuc
Copy link
Member

Gnimuc commented Mar 7, 2021

With JuliaInterop/Clang.jl#278, #39 has:

struct VkClearColorValue
    data::NTuple{16, UInt8}
end

function Base.getproperty(x::Ptr{VkClearColorValue}, f::Symbol)
    f === :float32 && return Ptr{NTuple{4, Cfloat}}(x + 0)
    f === :int32 && return Ptr{NTuple{4, Int32}}(x + 0)
    f === :uint32 && return Ptr{NTuple{4, UInt32}}(x + 0)
    return getfield(x, f)
end

function Base.getproperty(x::VkClearColorValue, f::Symbol)
    r = Ref{VkClearColorValue}(x)
    ptr = Base.unsafe_convert(VkClearColorValue, r)
    GC.@preserve r unsafe_load(getproperty(ptr, f))
end

function Base.setproperty!(x::Ptr{VkClearColorValue}, f::Symbol, v)
    unsafe_store!(getproperty(x, f), v)
end

@Gnimuc
Copy link
Member

Gnimuc commented Mar 7, 2021

Any comments on these Base.getproperty/Base.setproperty! methods?

@serenity4
Copy link
Member Author

serenity4 commented Mar 7, 2021

Looks neat! I don't know if it's a copy-pasting error, but you're missing a Ptr on the VkClearColorValue in ptr = Base.unsafe_convert(VkClearColorValue, r), I think it should be ptr = Base.unsafe_convert(Ptr{VkClearColorValue}, r).

It's type unstable, unfortunately, but since the property symbol is not known to the compiler, I don't think there is an easy workaround. Maybe one could make a macro for extracting the field, like @get x.float32, that way it can be made type stable never mind, it is actually type stable. Also, since the struct is just a bunch of bits, it may be nice to add a constructor that transforms any of the union types into bits, like

function VkClearColorValue(data::T) where {T<:Union{NTuple{4,Float32},NTuple{4,Int32},NTuple{4,UInt32}}}
    r = Ref(data)
    ptr = Base.unsafe_convert(Ptr{T}, r)
    ptr_uint8 = convert(Ptr{UInt8}, ptr)
    GC.@preserve r begin
        bits = unsafe_wrap(Vector{UInt8}, ptr_uint8, (16,))
    end
    VkClearColorValue(tuple(bits...))
end

@Gnimuc
Copy link
Member

Gnimuc commented Mar 7, 2021

Thanks! That's indeed a bug.

As for the constructor, I was thinking to generate something like:

const __U_VkClearColorValue  = Union{NTuple{4,Float32},NTuple{4,Int32},NTuple{4,UInt32}}
VkClearColorValue(data::__U_VkClearColorValue) = VkClearColorValue(reinterpret(NTuple{16, UInt8}, [data])[])

@serenity4
Copy link
Member Author

It's more than 10 times faster than my version. Awesome!

@maj0e
Copy link

maj0e commented Jun 19, 2021

I have a similar issue with VkAccelerationStructureGeometryDataKHR, which is specified like following:

// Provided by VK_KHR_acceleration_structure
typedef union VkAccelerationStructureGeometryDataKHR {
    VkAccelerationStructureGeometryTrianglesDataKHR    triangles;
    VkAccelerationStructureGeometryAabbsDataKHR        aabbs;
    VkAccelerationStructureGeometryInstancesDataKHR    instances;
} VkAccelerationStructureGeometryDataKHR;

But is in VulkanCore defined like this:

struct VkAccelerationStructureGeometryDataKHR
    triangles::VkAccelerationStructureGeometryTrianglesDataKHR
end

which means I'm at the moment not able to generate a TopLevel AccelerationStructure from instanceData.
Would that be solved be the approach described above or should I open another issue?

@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

Which version are you using? Is this union in a Vulkan extension? I can not find the definition.

@maj0e
Copy link

maj0e commented Jun 20, 2021

I'm using VulkanCore v1.2.3 through Vulkan.jl. I checked VulkanCore#master directly, though, which has the same type definition.
The VkAccelerationStructureGeometryDataKHR type belongs to the VK_KHR_acceleration_structure extension. The definition for the type is for me located at VulkanCore/oRcIR/gen/vk_common.jl:9190.

VulkanCore.LibVulkan.VkAccelerationStructureGeometryDataKHR(triangles::VulkanCore.LibVulkan.VkAccelerationStructureGeometryTrianglesDataKHR) in VulkanCore.LibVulkan at $HOME/.julia/packages/VulkanCore/oRcIR/gen/vk_common.jl:9190

@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

There is no vk_common.jl on VulkanCore.jl's master branch.

VK_KHR_acceleration_structure is not in this table, so VulkanCore.jl does not provide it.

It's probably in the Vulkan beta API. As discussed in #43, we need another package for these beta API definitions.

cc @serenity4

@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

This issue has already been fixed by #39. Let's continue the discussion in #43.

@Gnimuc Gnimuc closed this as completed Jun 20, 2021
@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

OK, it's not in the beta API and should be generated in the lib folder.

@serenity4
Copy link
Member Author

VK_KHR_acceleration_structure has been promoted to core as of version 1.2.177 (the one we're using), so its structs should definitely be available. I suspect we used the wrong version for the previous code generation by accident.

@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

Yeah, I'm going to push a fix on the master branch and tag v1.2.5.

@maj0e
Copy link

maj0e commented Jun 20, 2021

I'm using VulkanCore v1.2.3 through Vulkan.jl. I checked VulkanCore#master directly, though, which has the same type definition.

Ok this was my bad. I checked VulkanCore#master again and it's just like you described. Sorry for the false information.

Yeah, I'm going to push a fix on the master branch and tag v1.2.5.

Thank you!

@Gnimuc
Copy link
Member

Gnimuc commented Jun 20, 2021

@maj0e as Vulkan.jl still uses v1.2.3, I guess you could directly search and copy-paste the definitions you need to make an ad-hoc fix.

struct VkAccelerationStructureGeometryDataKHR
data::NTuple{64, UInt8}
end
function Base.getproperty(x::Ptr{VkAccelerationStructureGeometryDataKHR}, f::Symbol)
f === :triangles && return Ptr{VkAccelerationStructureGeometryTrianglesDataKHR}(x + 0)
f === :aabbs && return Ptr{VkAccelerationStructureGeometryAabbsDataKHR}(x + 0)
f === :instances && return Ptr{VkAccelerationStructureGeometryInstancesDataKHR}(x + 0)
return getfield(x, f)
end
function Base.getproperty(x::VkAccelerationStructureGeometryDataKHR, f::Symbol)
r = Ref{VkAccelerationStructureGeometryDataKHR}(x)
ptr = Base.unsafe_convert(Ptr{VkAccelerationStructureGeometryDataKHR}, r)
fptr = getproperty(ptr, f)
GC.@preserve r unsafe_load(fptr)
end
function Base.setproperty!(x::Ptr{VkAccelerationStructureGeometryDataKHR}, f::Symbol, v)
unsafe_store!(getproperty(x, f), v)
end
struct VkAccelerationStructureGeometryKHR
sType::VkStructureType
pNext::Ptr{Cvoid}
geometryType::VkGeometryTypeKHR
geometry::VkAccelerationStructureGeometryDataKHR
flags::VkGeometryFlagsKHR
end

These are the definitions for aarch64-linux-gnu, you might need to choose another one that matches your local machine. But in most cases, these definitions should be the same.

@serenity4
Copy link
Member Author

I am planning to update Vulkan.jl to the latest version of VulkanCore for the next release, along with a few breaking changes (hopefully not too many), and there are a couple of things that require some work before it is ready.

In the meantime, you can try a manual patch, but this may break the behavior of the wrapper for Vulkan.jl for this particular struct. Therefore, feel free to use VulkanCore directly as shown above. To complete what @Gnimuc said, if you don't want to modify VulkanCore directly you can define your own type (copy-pasting the code above) and ccall into it directly.

You can check out this section and this section of the Vulkan.jl documentation for some help if needed.

@maj0e
Copy link

maj0e commented Jun 20, 2021

Thanks, both of you. I was able to make it work by using VulkanCore directly as described above. This is a site project for me to learn about the new raytracing extensions, so I'm in no hurry and will probably wait for the new Vulkan.jl version before proceeding with the raytracing_pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants