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

Change how device address is requested to avoid future API breakage #101561

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Jan 14, 2025

PR #100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT.

However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address".

This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag.

This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5.

Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof.

This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms.

This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.

@darksylinc darksylinc requested a review from a team as a code owner January 14, 2025 23:08
@akien-mga akien-mga added this to the 4.4 milestone Jan 14, 2025
@darksylinc darksylinc force-pushed the matias-device-address-api branch from 021f86b to e43ce4f Compare January 15, 2025 15:14
@darksylinc darksylinc requested a review from a team as a code owner January 15, 2025 15:14
@darksylinc darksylinc force-pushed the matias-device-address-api branch from e43ce4f to 4ba2fad Compare January 15, 2025 15:24
@darksylinc darksylinc requested review from a team as code owners January 15, 2025 15:24
@darksylinc darksylinc force-pushed the matias-device-address-api branch 3 times, most recently from 281ab16 to 1d4c573 Compare January 15, 2025 19:41
@darksylinc
Copy link
Contributor Author

In the XML docs, I had to mix tabs & spaces instead of only tabs so that the code would be properly indented in Godot.

If I used pure tabs, the doctool would undo the indentation, showing this:

rd = RenderingServer.get_rendering_device()

if rd.has_feature(RenderingDevice.SUPPORTS_BUFFER_DEVICE_ADDRESS):
storage_buffer = rd.storage_buffer_create(bytes.size(), bytes, RenderingDevice.STORAGE_BUFFER_USAGE_SHADER_DEVICE_ADDRESS):
storage_buffer_address = rd.buffer_get_device_address(storage_buffer)

Instead of this:

rd = RenderingServer.get_rendering_device()

if rd.has_feature(RenderingDevice.SUPPORTS_BUFFER_DEVICE_ADDRESS):
	  storage_buffer = rd.storage_buffer_create(bytes.size(), bytes, RenderingDevice.STORAGE_BUFFER_USAGE_SHADER_DEVICE_ADDRESS):
	  storage_buffer_address = rd.buffer_get_device_address(storage_buffer)

image

@akien-mga
Copy link
Member

In the XML docs, I had to mix tabs & spaces instead of only tabs so that the code would be properly indented in Godot.

That's by design (for now). The XML structure should be indented with tabs, and code in codeblocks should be indented with 4 spaces. They're two different contexts using a different indent convention, albeit an arguably weird one.

We'll probably change this with #89819 but for now you need to follow the current convention.

@Repiteo Repiteo modified the milestones: 4.4, 4.x Jan 20, 2025
@clayjohn clayjohn modified the milestones: 4.x, 4.4 Jan 24, 2025
@Repiteo Repiteo requested a review from DarioSamo January 24, 2025 20:24
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested change is a good idea, but there's some inconsistency with the usage of bits in this class when we usually refer to them as "flags" in any other function on it. If the terminology is fixed I think this is good to go if it's given some testing.

@darksylinc
Copy link
Contributor Author

You mean BUFFER_CREATION_DEVICE_ADDRESS_BIT should be BUFFER_CREATION_DEVICE_ADDRESS_FLAG and BufferCreationBits should be BufferCreationFlags ?

@DarioSamo
Copy link
Contributor

DarioSamo commented Feb 4, 2025

You mean BUFFER_CREATION_DEVICE_ADDRESS_BIT should be BUFFER_CREATION_DEVICE_ADDRESS_FLAG and BufferCreationBits should be BufferCreationFlags ?

Only the latter change, you shouldn't need to specify _BIT or _FLAG on the enum itself.

@darksylinc
Copy link
Contributor Author

Only the latter change, you shouldn't need to specify _BIT or _FLAG on the enum itself.

Wait. I'm confused. A lot of other code in the codebase uses that convention. e.g.

enum TextureAspectBits {
	TEXTURE_ASPECT_COLOR_BIT = (1 << TEXTURE_ASPECT_COLOR),
	TEXTURE_ASPECT_DEPTH_BIT = (1 << TEXTURE_ASPECT_DEPTH),
	TEXTURE_ASPECT_STENCIL_BIT = (1 << TEXTURE_ASPECT_STENCIL),
};

enum PipelineStageBits {
	PIPELINE_STAGE_TOP_OF_PIPE_BIT = (1 << 0),
	PIPELINE_STAGE_DRAW_INDIRECT_BIT = (1 << 1),
	// ...
};

enum BarrierAccessBits {
	BARRIER_ACCESS_INDIRECT_COMMAND_READ_BIT = (1 << 0),
	BARRIER_ACCESS_INDEX_READ_BIT = (1 << 1),
	// ...
};

enum WindowFlagsBit {
	WINDOW_FLAG_RESIZE_DISABLED_BIT = (1 << WINDOW_FLAG_RESIZE_DISABLED),
	WINDOW_FLAG_BORDERLESS_BIT = (1 << WINDOW_FLAG_BORDERLESS),
	// ...
};

@akien-mga
Copy link
Member

akien-mga commented Feb 5, 2025

It seems the usage is currently inconsistent between Flags vs Bits suffix for the enum, and _BIT or nothing for the actual flags. So I'd just align with whatever seems to be used in this class / related classes. From the above there seems to be a fair bit of Bits/_BIT in rendering.

@DarioSamo
Copy link
Contributor

Let's go with Bits for the enum and _BIT for the enum itself.

@darksylinc darksylinc force-pushed the matias-device-address-api branch from 1d4c573 to 8d20b35 Compare February 11, 2025 22:52
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT.

However it did so by adding a boolean to uniform_buffer_create(), called
"bool p_enable_device_address".

This makes maintaining backwards compatibility harder because I am
working on another feature that would require introducing yet another
bit flag.

This would save us the need to add fallback routines when the feature I
am working on makes it to Godot 4.5.

Even if my feature doesn't make it to 4.5 either, this PR makes the
routine more future-proof.

This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into
BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to
both storage and uniforms.

This PR also moves the boolean use_as_storage into
BUFFER_CREATION_AS_STORAGE.
@darksylinc darksylinc force-pushed the matias-device-address-api branch from 8d20b35 to af900a5 Compare February 11, 2025 23:00
@darksylinc
Copy link
Contributor Author

All checks passed!

@akien-mga akien-mga requested a review from DarioSamo February 12, 2025 09:31
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

It is a bit awkward to break compatibility in the late beta stages. But we agreed we wanted to break compatibility now so we don't have to do it next release. From the perspective of users updating from 4.3 to 4.4 we will only break compatibility once anyway.

@Repiteo Repiteo merged commit 1939e87 into godotengine:master Feb 12, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 12, 2025

Thanks!

@adamscott
Copy link
Member

This PR caused a huge regression unfortunately: #102791
cc. @darksylinc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Not Critical
Development

Successfully merging this pull request may close these issues.

6 participants