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

Add custom texture create function #79288

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jul 10, 2023

This PR adds a method to RenderingServer, texture_rd_create that allows you to create a texture object using a RID relating to a texture created directly with the RenderingDevice. This in turn allows you to use that texture object to use the texture for materials and such.

For those who want to test, a little example project I wipped together:
TestCustomTextures.zip

Looks way better animated but it implements an old fashion ripple effect as a compute shader:
image

@clayjohn
Copy link
Member

CC @joemarshall

texture.rd_texture_srgb = RD::get_singleton()->texture_create_shared(rd_view, p_rd_texture);
}

// TODO figure out what to do with slices
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea if we need to populate this data...

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looks good, just the comments noted (also check comment about freeing the RD RID above).

scene/resources/texture.h Outdated Show resolved Hide resolved
@sakrel
Copy link
Contributor

sakrel commented Jul 11, 2023

Does this play nice when RenderingServer is in multi-threaded mode?
Imagine you create a texture with RenderingDevice, you then

  1. access that texture with RenderingServer
  2. and after that access the texture with RenderingDevice again.

It's my understanding that when RenderingServer is in multi-threaded mode, RenderingServer commands are queued and then executed at a later point on another thread. How do you make sure that RenderingServer commands in step 1) execute before step 2)?

Basically, what I'm worried about is that when you turn multi-threaded mode on, code that worked perfectly fine in single-threaded mode suddenly stops working.

@clayjohn
Copy link
Member

@sakrel Ultimately the RenderingServer is calling into the RenderingDevice to handle the texture. As all the RenderingServer deals with is the RID. Since the RenderingDevice is thread safe, everything should just work shouldn't it?

I should say "since the RenderingDevice is thread safe, in theory" Since we have a bit of work to do to make it more thread safe and efficient. Right now we just throw mutexes at the beginning of every method we want to be thread safe

@sakrel
Copy link
Contributor

sakrel commented Jul 11, 2023

@sakrel Ultimately the RenderingServer is calling into the RenderingDevice to handle the texture. As all the RenderingServer deals with is the RID. Since the RenderingDevice is thread safe, everything should just work shouldn't it?

Yes, the RenderingDevice itself should be fine. It's just that when the RenderingServer is multi-threaded, it's possible for function calls to be reordered. Take this code for example:

	var thread_model: int = ProjectSettings.get_setting("rendering/driver/threads/thread_model");
	if thread_model == 0:
		print("Thread Model: ", "Single-Unsafe");
	elif thread_model == 1:
		print("Thread Model: ", "Single-Safe");
	elif thread_model == 2:
		print("Thread Model: ", "Multi-Threaded");
		
	
	var rd := RenderingServer.get_rendering_device();
	var format := RDTextureFormat.new();
	format.format = RenderingDevice.DATA_FORMAT_R8G8B8A8_UNORM;
	format.texture_type = RenderingDevice.TEXTURE_TYPE_2D;
	format.usage_bits = RenderingDevice.TEXTURE_USAGE_SAMPLING_BIT | RenderingDevice.TEXTURE_USAGE_CAN_UPDATE_BIT | RenderingDevice.TEXTURE_USAGE_CAN_COPY_FROM_BIT | RenderingDevice.TEXTURE_USAGE_CAN_COPY_TO_BIT;
	format.width = 1;
	format.height = 1;
	var rd_texture := rd.texture_create(format, RDTextureView.new());
	var texture := RenderingServer.texture_rd_create(rd_texture);
		
	rd.texture_clear(rd_texture, Color.WHITE, 0, 1, 0, 1);
	
	var image :=  Image.create(format.width, format.height, false, Image.FORMAT_RGBA8);
	image.fill(Color.BLACK);
	RenderingServer.texture_2d_update(texture, image, 0);

	var data := rd.texture_get_data(rd_texture, 0);
	print("texture_get_data Color: ", data);

So, first we use RenderingDevice::texture_clear to clear the RenderingDevice texture to white, then we use RenderingServer::texture_2d_update to clear the texture to black.

With thread model Single-Safe, I get the following results:

Thread Model: Single-Safe
texture_get_data Color: [0, 0, 0, 255]

As expected, the texture color is black.
When I run the same code with thread model Multi-Threaded, the texture color is white:

Thread Model: Multi-Threaded
texture_get_data Color: [255, 255, 255, 255]

I think this happens because the engine doesn't execute RenderingServer.texture_2d_update(texture, image, 0); immediately. Instead, the RenderingServer.texture_2d_update command is enqueued and executed at a later point by the RenderingServer thread.

@joemarshall
Copy link
Contributor

Like this pr, made a few comments.

About threading, nothing quite works right now - I've got a work in progress to threadify renderingdevice - there's a few API changes needed which is annoying - most obviously draw_list_end which doesn't take a draw list ID, along with the various message things which underlying are using a draw list but without taking an id.

@BastiaanOlij
Copy link
Contributor Author

@sakrel I think that use case would always result in something unexpected, at some point we need draw a line whether users are doing something logical or are just trying to trip up the system. My only "fear" in this is that documentation may not be clear and people take an overcomplicated approach because they are looking at two completely unrelated examples and trying to merge them.

The whole premise is that you will only work with RenderingDevice directly when you want to create your own systems for rendering too textures, be it compute shaders or through framebuffers where you're rendering stuff that viewports aren't suitable for.

So it will always be through RenderingDevice that you are populating the texture. The RenderingServer texture is purely used for consuming that data, being able to display the contents of the texture or use it in materials.

Having a scenario where both RD and RS are writing to the texture and doing things that contradict each other, you get into trouble. For instance for your example texture_2d_update, you're actually not rendering to the RD texture, you're replacing it with a new RD texture object created from the image you supplied.

The docs must be clear on this, you can't interchange texture_2d_..., texture_3d_..., texture_rd_... functions.

@clayjohn
Copy link
Member

@BastiaanOlij I think the issue @sakrel raises is an important one for us to consider. There is also one extra complication. While the texture_**_update() functions are queued up for thread safety, the texture_**_create() functions bypass the queue and can execute directly. See:

#define FUNCRIDTEX0(m_type) \
virtual RID m_type##_create() override { \
RID ret = RSG::texture_storage->texture_allocate(); \
if (Thread::get_caller_id() == server_thread || RSG::texture_storage->can_create_resources_async()) { \
RSG::texture_storage->m_type##_initialize(ret); \
} else { \
command_queue.push(RSG::texture_storage, &RendererTextureStorage::m_type##_initialize, ret); \
} \
return ret; \
}

This means that creating a texture from the RenderingServer and accessing it from the RenderingDevice will appear to work fine, while updating it will break.

I think we need to explore whether we can make the texture_**_update() functions thread safe in the way that the texture_**_create() functions are so that users won't get surprised by this.

If that isn't possible, then we need to have clear warnings in the docs pages for any functions in either API that may suffer from this issue

@BastiaanOlij
Copy link
Contributor Author

@clayjohn don't they all work the same? The only thing texture_2d_create does outside of the thread is reserve the RID for the texture, it doesn't actually populate the texture structure nor create anything in the rendering servers until texture_2d_instantiate is called, which has the same thread protection code as texture_2d_update.

But I was partly wrong in what I said, you can populate and rd texture with texture_2d_update. But assuming texture_rd_create and texture_2d_update are called from the same thread, they will either both be executed right away (if we're on the main thread) or both queued (if we're not on the main thread). It's only the reserve of the RID that happens right away which is important as that is our denominator.

@clayjohn
Copy link
Member

@BastiaanOlij Take a look at the code block I posted above. texture_2d_create() calls texture_2d_instantiate() immediately when using the RD renderer. It is only queued when using the GLES3 renderer

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 12, 2023

@BastiaanOlij Take a look at the code block I posted above. texture_2d_create() calls texture_2d_instantiate() immediately when using the RD renderer. It is only queued when using the GLES3 renderer

Ah because of the can_create_resources_async?

Hmm, so if I understand correctly, the problem is that if you're not on the main thread, and you call texture_2d_update, that is now queued. When you then try to access the RD texture, depending on what texture_2d_update does, you may be getting the value as it was before the change is applied, not after.

I think we have that problem in many places, basically an object should be marked pending if it's added stuff to the command queue and any getter that accesses data we get from the render device in the queued method, should yield the thread to let anything on the command queue be processed by the main thread before continuing.

@joemarshall
Copy link
Contributor

Currently when you call texture_get_data, there's a call to flush, which flushes all draw calls, so in theory it should get everything that is drawn on whatever thread. Although it is a total pipeline stall which means that anything using texture_get_data is a bad idea anyway; that needs deprecating and replacing with async texture fetchers some point soon, or to return some kind of future-like object or something.

The problem being observed above isn't anything to do with renderingdevice though, it is to do with the renderingserver proxying stuff to the rendering thread using command queue. Which whilst it makes sense for the GL renderer, needs some consideration for vulkan as to whether it is still appropriate.

My instinct on this is that:
a) RenderingServer should proxy things to the renderer thread so it is easy to understand and use. This should include everything except for resource creation calls I think, although I don't quite know how much performance benefits creating resources on multiple threads actually gives, and as you see above, it creates confusion at points.

b) renderingdevice should support writing draw buffers, updating textures etc. and everything on multiple threads.

c) Consistency within threads should exist, i.e. if you call something to draw into a texture on a thread, then call something to read that texture on the same thread, you should get the right answer.

d) The order of GPU operations between threads should not necessarily be guaranteed - that way we only have to flush everything at the end of frame, rather than introducing dependencies between threads.

e) We should really really strongly discourage combination of renderingserver and renderingdevice calls like the above, except where absolutely required during the process of transferring resources from one to the other. It is totally asking for trouble...

What this basically means from a vulkan point of view is that there is a separate vulkan command list for each thread. All command lists are transferred on frame end as they are currently. As we primarily use one queue for drawing, texture uploads etc, within thread consistency should be fine. texture_get_data etc. should firstly be discouraged, but secondly should push all pending commands (or maybe just the ones pending on that thread?) including the commands to copy data back, protected by appropriate barriers, then wait on a fence, as opposed to the current situation which is that it waits until everything pending is processed and the GPU is idle. Which as I mention above is BAD.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 15, 2023

Incidentally, what is the main thread here depends on the rendering setting. When multithreaded rendering is turned on, godotscript is never running on the rendering thread, so if renderingserver calls are queued, renderingdevice calls will do their thing before the renderingserver call gets a chance to run the queued calls. All the more reason why the renderingdevice calls should never be mixed with renderingserver except in very specific circumstances.

I'm not sure about this btw, I know this is so in Godot 3, but in Godot 4 we're using threading in a very different way. I've never been very confident about the way we're handling this in Godot 3 but I also have to admit I never investigated it thoroughly.

The only reason you'd want rendering to run on another thread is that you want to start processing the next frame, but you really can't do that until after you've made a snapshot of current instance data, which is something we do while looping through our viewports.

Our current approach of using threading in the rendering server itself controlled from the main thread seems a much better approach, where we efficiently handle the current frame, build the command buffers, and then sent them off to the GPU, the GPU can then do its work in parallel with us processing the next frame (which now also makes use of multi threading to paralyze a lot of the work).

Having a separate rendering thread seems like little win while complicating matter and requiring some serious synching anyway.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review July 20, 2023 05:38
@BastiaanOlij BastiaanOlij requested review from a team as code owners July 20, 2023 05:38
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 20, 2023

I've moved this out of draft and added an example project so people can start playing with it.

I do not know what I did that caused the CI to fail on documentation on texture classes this PR doesn't touch.

edit owh I forgot to push in my rebase and latest fixes, maybe that clears it up 🤞

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.

Amazing work! I think this is nearly ready to go. I left a few comments and suggestions. The biggest missing work item is to add support for Cubemaps and Cubemap Arrays on the RS side. Other than that I think this is ready to go.

I tested it locally with your attached demo and it works great. We should get your demo uploaded to the demos repo as soon as we merge this

drivers/vulkan/rendering_device_vulkan.cpp Outdated Show resolved Hide resolved
servers/register_server_types.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_rd/scene/texture_rd.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_rd/scene/texture_rd.cpp Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/TextureLayeredRD.xml Outdated Show resolved Hide resolved
doc/classes/TextureLayeredRD.xml Outdated Show resolved Hide resolved
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 24, 2023

Demo has now been added to the demo projects: godotengine/godot-demo-projects#938

@BastiaanOlij BastiaanOlij force-pushed the custom_texture_api branch 2 times, most recently from b94c15a to 6e4c47c Compare July 26, 2023 04:20
@YuriSizov YuriSizov merged commit d49ea2b into godotengine:master Jul 26, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@krisp-xyz
Copy link

krisp-xyz commented Sep 6, 2023

hi! I tried following the example and ran into this error: Trying to assign value of type 'Compressed2D' to a variable of type 'Texture2DRD'.
I'm sure it's a user error since your example works fine, but I can't figure out what the difference in my code that causes the error is.

texture is a Texture2DRD and tex is a sampler2D in the gdshader

if material:
	texture = material.get_shader_parameter("tex")
	if texture:
		texture.texture_rd_rid = output_tex

@critopadolf
Copy link

Can hint_screen_texture or a viewport be used as a Texture2drd?

Viewport texture rid is seen as invalid by rd:

using Godot;
using System.Diagnostics;
public partial class minimum : SubViewport
{
    private RenderingDevice rd;
    public override void _Ready()
    {
        rd = RenderingServer.GetRenderingDevice();
        Debug.Assert(rd.TextureIsValid(this.GetTexture().GetRid()));
    }
}

and I'm not sure how to get hint_screen_texture's Rid (assuming there is one)

Thanks!

@clayjohn
Copy link
Member

clayjohn commented Nov 7, 2023

Can hint_screen_texture or a viewport be used as a Texture2drd?

The screen texture definitely cannot be used as a Texture2DRD. I don't think we allow using a Viewport's texture either. Texture2DRD is a texture resource, it fills a very different purpose from Viewports

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

Successfully merging this pull request may close these issues.

8 participants