Replace use of push constants in 2D RD renderer with uniform buffer

[SlugFiller]

This removes the 128 byte limitation on push constant size, by only using the push constant as an index into the generated buffer. This allows adding more per-instance per-draw command data.

[AThousandShips]

Suggested change

	return; //nothing to do, its the same
	return; // Nothing to do, it's the same.

Comment style nitpick, applies to all modified code even if it is just copied or relocated

[akien-mga]

Also, "its" -> "it's"

[AThousandShips]

Suggested change

	//something odd happened
	// Something odd happened.

[AThousandShips]

Suggested change

	//bind pipeline
	// Bind pipeline.

[AThousandShips]

Suggested change

	//bind textures
	// Bind textures.

[AThousandShips]

Suggested change

	//bind pipeline
	// Bind pipeline.

[AThousandShips]

Suggested change

	//pass collision information
	// Pass collision information.

[AThousandShips]

Suggested change

	fb_uniform_set = RID(); // Force the uniform set to be recreated with the new buffer
	fb_uniform_set = RID(); // Force the uniform set to be recreated with the new buffer.

[AThousandShips]

Suggested change

	//push constants buffer
	// Push constants buffer.

[AThousandShips]

Suggested change

	inline void _prepare_canvas_texture_data(RID p_texture, RS::CanvasItemTextureFilter p_base_filter, RS::CanvasItemTextureRepeat p_base_repeat, RID &r_last_texture, PushConstant &push_constant, Size2 &r_texpixel_size); //recursive, so regular inline used instead.
	inline void _prepare_canvas_texture_data(RID p_texture, RS::CanvasItemTextureFilter p_base_filter, RS::CanvasItemTextureRepeat p_base_repeat, RID &r_last_texture, PushConstant &push_constant, Size2 &r_texpixel_size); // Recursive, so regular inline used instead.

[AThousandShips]

Suggested change

	inline void _bind_canvas_texture(RD::DrawListID p_draw_list, RID p_texture, RS::CanvasItemTextureFilter p_base_filter, RS::CanvasItemTextureRepeat p_base_repeat, RID &r_last_texture, const PushConstant &push_constant); //recursive, so regular inline used instead.
	inline void _bind_canvas_texture(RD::DrawListID p_draw_list, RID p_texture, RS::CanvasItemTextureFilter p_base_filter, RS::CanvasItemTextureRepeat p_base_repeat, RID &r_last_texture, const PushConstant &push_constant); // Recursive, so regular inline used instead.

[SlugFiller]

Fixed comment styling.

P.S. If it's a priority, shouldn't there be a mega-PR fixing comment styling across the entire codebase?

[AThousandShips]

That's why we do it like this, to bring it slowly in line with the correct format, instead of doing a massive PR polluting the blame history, if your code already touches it however it's different, see #80414 (comment)

[clayjohn]

This is a great start! Have you tested performance at all vs using push constants? I suspect that this approach is around the same speed as the current approach, but it might be slightly slower. At any rate, it is a good start to build off of.

On that note, what are your plans for moving forward with this? Are you going to take a stab at batching next, or are you going to look into per-instance uniforms?

[SlugFiller]

@clayjohn For now I'm leaving it as is. I'm just creating it as a basis for others. I have other priorities for next features.

If I was doing something extra, it would have been color/modulation separation. But once this is merged, it should be a small enough change that anyone could do it.

[clayjohn]

@clayjohn For now I'm leaving it as is. I'm just creating it as a basis for others. I have other priorities for next features.

If I was doing something extra, it would have been color/modulation separation. But once this is merged, it should be a small enough change that anyone could do it.

I see. I guess I misunderstood the plan for this. As-is, I don't think we should merge this change by itself. Right now it adds complexity to the renderer for no apparent gain. While it is helpful foundational work for other possible features/improvements, I think it is better to leave this PR open for other potential contributors to pick up where you left off

[SlugFiller]

@clayjohn I suppose it's possible for others to build PRs on top of an unmerged PR. I probably wouldn't. But it's your call. This one isn't really a blocker for me.

Plus, it would be easier for me to rebase this on top of canvas groups rather than the other way around. So I'd rather that one be merged first anyway.

[Calinou]

As I understand it, this PR makes it possible to address #49781 in the future.

[darksylinc]

I agree with the author that reading draw_data_buffer.data[draw_data_index.index] all the time instead of simply data makes parsing the code ('parsing' as in mentally parsing and understanding what is going on, I'm not talking about compilers) much harder.

As I just replied on Godot chat, there are two possible ways:

1. Using #define myStruct draw_data_buffer.data[draw_data_index.index] as he pointed out.
2. Simply copy by value const MyStruct myStruct = draw_data_buffer.data[draw_data_index.index]

As long as data comes from an UBO, copy by value is free because the shaders, unlike C code, can assume that the pointer is read-only and does not alias (see strict aliasing in C).

That is a bit more complicated if draw_data_buffer is a non-read-only non-restrict SSBO, but that is rare (and usually kills performance regardless of what you do).

If Copy-by-value causes a performance degradation then that's more likely to have to do with glslang compiler not being good enough at optimizing this case, rather than a design/pattern problem.

[SlugFiller]

@Calinou You understand correctly. That is the main point. It was theoretically possible to address this in a single mega-PR, but it feels like two separate changes to me, so I only created the foundation, leaving the rest for a separate PR.

Another big reason to not make it in a single PR, is that there are multiple possible ways to solve #49781 (building on this PR), with some being potentially breaking. So this PR only does the change that would be common to any such solution, and leaves the bikeshedding for a later date.

I took @darksylinc 's advice and minimized the changes to canvas.glsl. It is a lot more readable now, and isn't as harmful to merges and the blame history.

[BastiaanOlij]

Approved in principal pending two point discussed during rendering effects:

• rename to something more appropriate such as PerInstanceBuffer
• have a use case implemented so it is not just a structural place

[SlugFiller]

Renamed and rebased.

[nikitalita]

In my testing with games that have huge amount of sprites on the screen (bullet hell games, like this one), this PR performed slightly faster than current master; around a 4-5% speed-up.