Split raster barrier into vertex and fragment barrier

[BastiaanOlij]

This PR splits the raster barrier into a vertex and fragment barrier. This is extracted from #76872

For the Forward+ renderer this distinction does not matter. The vertex and fragment shader is executed in parallel. This chance will be transparent as the raster barrier still works as before.

For mobile GPUs this distinction can make a performance improvement as the TBDR architecture results in the vertex and fragment shader being executed mostly in sequence. This means that if a fragment shader is dependent on the output of a fragment shader of a previous pass, the vertex shader can run in parallel with the fragment shader of the previous pass. Similarly if we need to populate a UBO this can happen while the previous fragment shader is active.

The main improvement will thus be applying this and finding places in the code where we can apply Fragment -> Fragment barriers and make use of this ability to overlap.

This PR applies this in one place, when rendering cubemap shadows. In this case we're rendering our shadows to 6 sides of a cube map, and then copy that result into our shadow atlas.

With the current barrier the cubemap must fully finish before we start processing the copy, with the new approach we can have some minor overlap.

There are further improvements that we can do in a follow up once other changes are applied as well. For instance rendering of shadowmaps can have a better overlap of running the next pass while the previous pass fragment shader is still running.

[clayjohn]

Looks great! This will be super helpful for the mobile renderer

[clayjohn]

Before merging we need to discuss the compatibility breakage issue.

My understanding is that this won't break compatibility as existing projects referencing a BarrierMask will continue to behave the same, its just the internal bits that have changed. The only situation where things could conceivably break is if someone is using raw bits as a bit field and not using the enums, but I am not sure that is a use-case worth worrying about

[BastiaanOlij]

Before merging we need to discuss the compatibility breakage issue.

I was thinking about this too. I think the risk at the moment is really tiny, we don't expose this to GDExtension yet which would be my main worry.

That said, this isn't very future proof. I'm wondering if we shouldn't change BARRIER_MASK_ALL_BARRIERS to 0x7FFFFFFF and set BARRIER_MASK_NO_BARRIER to 0x80000000 or something like this so that if we introduce new barriers (maybe for indirect draw at some point) we don't get these massive changes.

(Note for those who may wonder about this. BARRIER_MASK_NO_BARRIER isn't set to 0 as it's a special case we identify to skip over calling the barrier code all together, at least thats the impression I have. Could be worth adding as a comment)

[RandomShaper]

If anoyne is using hardcoded values, they are doomed to failure anyway. That said, the switch to an all-ones all barriers mask value is a nice idea.

[RandomShaper]

This should be fine, but maybe can be merged after 4.1 for extra safety?

[BastiaanOlij]

This should be fine, but maybe can be merged after 4.1 for extra safety?

Yeah 100% agree, there is no need for this to be in 4.1

[YuriSizov]

Needs a rebase. Also would appreciate a "go ahead" from @clayjohn

[YuriSizov]

@BastiaanOlij Poke poke for a rebase :)

[YuriSizov]

I'm not sure I follow why this doesn't break compatibility, as this affects an exposed enum (and it affects the default value passed to some methods). E.g. BARRIER_MASK_COMPUTE has a different value now, so anything that deals with both identifiers/string representation and corresponding numeric values is going to be affected.

But you've discussed this and agreed that the change is fine. So I'm going to follow your decision. Just know that it's going to be on you to address potential compatibility issues should we face them 🙃

[YuriSizov]

Thanks!

[clayjohn]

I'm not sure I follow why this doesn't break compatibility, as this affects an exposed enum (and it affects the default value passed to some methods). E.g. BARRIER_MASK_COMPUTE has a different value now, so anything that deals with both identifiers/string representation and corresponding numeric values is going to be affected.

But you've discussed this and agreed that the change is fine. So I'm going to follow your decision. Just know that it's going to be on you to address potential compatibility issues should we face them upside_down_face

Looks like you are right. This PR is triggering our GDExtension compatibility checks in CI. We will need to add compatibility code if possible

[BastiaanOlij]

Just to recap a bit of the additional discussion around this.

Indeed, this caused issues with the CI checks, the enum changes are breaking and we didn't give that enough thought up front. That said, it's identified a problem with us exposing our entire API, even parts that are still seeing heavy development and more importantly, that aren't usable outside of the core but are purely exposed because we're preparing for when they do become usable.

This enum change was required because when the barrier system was designed, the requirements were based on desktop GPU architecture, where this change is completely unnecessary. But on mobile GPUs TBDR it is very important to be able to handle barriers at this granularity.

When we look at outside usage, there currently is none as we've yet to expose large parts of the rendering system to GDExtension to make it usable. So while this is a breaking change, it's unlikely to have actually broken something. It is highly unlikely anyone is using the barrier code from outside of Godot.

This claim does need to be verified however.

[YuriSizov]

Compatibility breakage in this case is not limited to GDExtension. It's just that only GDExtension has any real checks for compatibility at the moment. C# will soon have checks as well, but more importantly, our API surface is agnostic and this change modifies it regardless of specific API users. It can just as well affect some GDScript code, if someone relies on specific values and serializes them, for example.

[clayjohn]

@raulsntos Can you confirm that this would break C# scripts as well?

To be clear, this isn't a breaking change for GDScript as GDScript will use the enum values from the version of the engine that runs. The problem with GDExtension is that the enum values are baked into the GDExtension based on the version of the engine that the GDExtension targets.

Does C# do something like that? Or will it dynamically use the up-to-date enum values?

[raulsntos]

The problem with GDExtension is that the enum values are baked into the GDExtension based on the version of the engine that the GDExtension targets.

That's essentially how it works for C# as well. Built assemblies that targeted a previous version of GodotSharp will continue to use the old enum values.

This means that if you built a library that used BARRIER_MASK_RASTER in 4.1, a game that uses that library in 4.2 will be using the value 1 whereas it should be using 3 now.

As you might imagine this is worse than removing a method because users won't get an exception, but their code will break in unexpected ways.

[YuriSizov]

To be clear, this isn't a breaking change for GDScript as GDScript will use the enum values from the version of the engine that runs.

It is a breaking change for the API. GDScript runtime may be unaffected, but user code written in any language, including GDScript, can be — if it relies on specific numeric values to correspond to specific human-readable values. Since BARRIER_MASK_COMPUTE has a different value now, if anyone, for example, serialized its value before, they won't be able to correctly read it now (they'll read it as BARRIER_MASK_FRAGMENT). Users don't even need to do it consciously. It can be a value of an exported property and the engine does the serialization.

[AThousandShips]

I don't think it's possible to add compatibility methods as it is currently implemented, I'd suggest keeping the lsb as BARRIER_RASTER_COMPAT, moving the fragment and vertex to the end to avoid renaming the others, and adding a compat method handling the old raster value, also NO_BARRIER is also conflicting now, so I'd suggest reserving the old value and making the new one at the msb for future proofing

I think a bit of clutter is worth it to prevent this major compat breaking change in a minor version, it's not like we're short on bits here and need to save

i.e.:

BARRIER_MASK_RASTER_COMPAT = (1 << 0),
BARRIER_MASK_COMPUTE = (1 << 1),
BARRIER_MASK_TRANSFER = (1 << 2),
BARRIER_MASK_NO_BARRIER_COMPAT = (1 << 3),
BARRIER_MASK_VERTEX = (1 << 4),
BARRIER_MASK_FRAGMENT = (1 << 5),
BARRIER_MASK_RASTER = BARRIER_MASK_VERTEX | BARRIER_MASK_FRAGMENT,
BARRIER_MASK_ALL_BARRIERS = BARRIER_MASK_RASTER | BARRIER_MASK_COMPUTE | BARRIER_MASK_TRANSFER,
BARRIER_MASK_NO_BARRIER = (1 << 31), // Or (1 << 30), unsure about signedness here, or just keep this one as before and put the others after it

[YuriSizov]

We can also accept that breaking change as a necessity, by the way. Which is why I agreed to the merge despite my doubts.

[BastiaanOlij]

@AThousandShips doing it that way would require a bunch of additional checks to be added in many places where barriers are implemented, converting old values to new. But it could be worth it.

Personally I feel that it isn't simply because it is incredibly unlikely someone has used this from an extension. GDScript maybe but GDScript will re-evaluate the enums when the scripts are parsed in runtime.