Fix Vulkan Instance initialized twice in ProjectDialog

[Gaktan]

Fixes #96722

I refactored a bit to be able to share as much code as possible with other systems that create a local device,

I first wanted to replace the create_local_rendering_device(), but since it's apparently used in GDScript, I did not know if it was possible to pass pointer reference arguments.

Note: this does not fix the leak that still exists in create_local_rendering_device(): #71003

[akien-mga]

This is a pre-existing issue, so maybe not blocking, but these includes break code encapsulation. The generic / abstract servers code shouldn't need to access driver code, and instead have virtual methods that drivers can implement.

Also, nitpicking, but if those includes are needed for now, they should come after the block of core/servers includes, by convention.

[Gaktan]

I'm trying to think of the best way to do this. Does it make sense to have this in the DisplayServer class?
Since the RenderServer doesn't have children class or API specific code.
I don't know the subtlety between the two classes. Does it even make sense for the RenderServer to have RenderingServer::create_local_rendering_device() to begin with ?

[clayjohn]

I think this is too much complexity just to solve #96722.

The core issue in #96722 is that creating a RenderingDevice (not a local rendering device) crashes on certain hardware when a RenderingDevice has already been created. The solution is to add a check at the top of can_create_rendering_device() that checks if we are already using the Compatibility renderer. If we are, then it continues and attempts to create a RenderingDevice, if not, it does an early out and returns true.

[Gaktan]

That's fair.
Although what can_create_rendering_device() is doing is essentially the same steps as creating a local rendering device (literally the same code). I just took the opportunity to cleanup a bit and unify code

[clayjohn]

That's fair. Although what can_create_rendering_device() is doing is essentially the same steps as creating a local rendering device (literally the same code). I just took the opportunity to cleanup a bit and unify code

Not exactly. can_create_rendering_device() doesn't create a local rendering device so the flow is very different. The new function also makes the user responsible for cleaning up the RenderingDevice and Context which is a big problem for a function like this.

Also, as a bonus, while looking at the code again, I realized that it can done very simply just by checking if the RenderingDevice singleton exists:

RenderingDevice *device = RenderingDevice::get_singleton();
if (device) {
	return true;
}

This is what create_local_rendering_device() does, which is why attempting to create one works for this PR.

[dhoverml]

@Gaktan can you please rebase?
I wanted to confirm that this fixes #96938, but I am getting a non-trivial merge conflict when I cherry pick.

[Gaktan]

@dhoverml Should be good now

[dhoverml]

Thanks @Gaktan .
It looks like the crash is fixed with this PR, however, creating new windows is very slow. It takes about 1 second to create a new window (click on + Create in the Project Manager, or open any menu in the editor when Single Window Mode is false).
I don't know if that is unrelated to this PR though.

[Gaktan]

@clayjohn I'll do the simple fix for now, and do the refactor in an other PR

[clayjohn]

Looks great, thank you!

[clayjohn]

Thanks @Gaktan . It looks like the crash is fixed with this PR, however, creating new windows is very slow. It takes about 1 second to create a new window (click on + Create in the Project Manager, or open any menu in the editor when Single Window Mode is false). I don't know if that is unrelated to this PR though.

That's unrelated to this PR.

If you are on Windows you can improve the speed of opening new windows a lot by switching to D3D12 instead of Vulkan

[clayjohn]

@clayjohn I'll do the simple fix for now, and do the refactor in an other PR

Sounds good!

[akien-mga]

Thanks!