godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.97k stars 21.16k forks source link

Decals using ViewportTexture are not updated #73400

Open maiself opened 1 year ago

maiself commented 1 year ago

Godot version

aa6ec763179e5bf1d1300aa72dc5f4172d810efa

System information

Arch Linux, Mesa Intel® UHD Graphics (CML GT2), Vulkan Forward+

Issue description

Decals using ViewportTexture are not updated when the viewport texture is updated.

Workarounds are possible, clearing Decal.texture_* properties and resetting causes an update. Some updates must be preformed twice a frame apart to have effect.

The workaround code can be found in character_ring_decal/character_ring_decal.gd file of the MRP.

The following error is also reported, but its unclear why:

Node not found: "SubViewport" (relative to "CharacterRingDecal").
ViewportTexture: Path to node is invalid.

Steps to reproduce

Minimal reproduction project

viewport-texture-decal-issue.zip

clayjohn commented 1 year ago

I'm not sure if this is something that we should support. Decals are added to a decal atlas so that you can have multiple decals per surface. That means when you create a decal you have to copy the texture over onto the texture atlas (potentially resizing the atlas in the process).

Since ViewportTextures can update every frame, you would potentially have to do that copy every single frame. I think it should be possible, but I am not sure it is something that we should support.

We have the same issue when using ViewportTextures as projector texture or with PointLight2Ds

maiself commented 1 year ago

I'm aware of the decal atlas and the potential pitfalls of updating it frequently. In my project I'm intentionally avoiding updating the decals whenever possible by using SubViewport.UPDATE_ONCE, caching the textures for reuse, and quantizing the shader parameters to reduce cache misses. Perhaps this is a more advanced usage.

The need for workarounds to force updates leaves things feeling very unrefined.

Maybe this could be properly supported, with clear warnings about performance pitfalls?

Or document as undefined behavior?

Or disallow ViewportTexture and Decal use, and maybe document some path for creating decals from a one off dynamic source.

maiself commented 1 year ago

On mobile, accidently hit close with comment while writing...

betalars commented 1 year ago

I'm not sure if this is something that we should support.

This is not Apple, this is open Source. Please: let people break stuff.

You can already disable updates on view-ports. I can even see a configuration warning being used, when a seemingly static viewport seems to be set to update when assigned to a ViewportTexture.

But 90% of the use cases I can imagine for assigning a viewport texture to a projection require the projection to be dynamic.

But right now it is clearly a bug. And I've just wasted 2 hours on figuring this out.

QbieShay commented 1 year ago

+1 to this. I will eventually need a way to animate decals in VFX. Maybe we can have mesh decals for these usecases.

Calinou commented 1 year ago

+1 to this. I will eventually need a way to animate decals in VFX. Maybe we can have mesh decals for these usecases.

There's a proposal for mesh decals, but it's unlikely to be implemented given the complexity (and given how it duplicates an existing system). Nothing prevents you from using quads (or procedurally generated meshes) closely attached to a surface, still.

CarpenterBlue commented 1 year ago

I am not sure #77585 should have been closed. It wasn't about making the decals work but making the un-supported textures unavailable to the decals as they are not planned feature. Maybe I could edit the original Issue to make that more clear as it still persists.

Calinou commented 1 year ago

We could change the property hint type to list all the underlying Texture2D classes that are supported, but I don't remember if it's possible to list multiple object types to choose from (so we can avoid the unsupported types).

AThousandShips commented 1 year ago

It should be possible, GPUParticles2D makes use of this

Bimbam360 commented 1 year ago

Chipping in, is there any reason we cannot have a toggle in editor (at least temporarily) for the light node of 'Static'/'Dynamic' with subsequent performance warning in popup text? I think the assumed behaviour of most users would be that SubViewport.UPDATE_ALWAYS should do this anyway surely.

This would at least expose the option to users while a better solution re. performance be found?

Calinou commented 1 year ago

Chipping in, is there any reason we cannot have a toggle in editor (at least temporarily) for the light node of 'Static'/'Dynamic' with subsequent performance warning in popup text? I think the assumed behaviour of most users would be that SubViewport.UPDATE_ALWAYS should do this anyway surely.

If we allow using ViewportTexture, I don't think this needs to be an option within Light3D or Decal. We should just document the performance caveats of using a ViewportTexture that uses the Always update mode with decals or light projectors.

clayjohn commented 1 year ago

Chipping in, is there any reason we cannot have a toggle in editor (at least temporarily) for the light node of 'Static'/'Dynamic' with subsequent performance warning in popup text? I think the assumed behaviour of most users would be that SubViewport.UPDATE_ALWAYS should do this anyway surely.

If we allow using ViewportTexture, I don't think this needs to be an option within Light3D or Decal. We should just document the performance caveats of using a ViewportTexture that uses the Always update mode with decals or light projectors.

I think a setting is still needed as the renderer needs to know to update that texture every frame. Right now the current workaround method may cause the decal atlas to shuffle everything every frame. But what we want is for the decal atlas to have a list of textures that it blits from each frame.

QbieShay commented 1 year ago

I'm thinking that rather than supporting viewport textures, we should perhaps think of more ad-hoc solutions for this. For example there can be in the rendering a special decal pass that outputs to the relevant part of the screen for the decal for all the buffers that decal support, and the shader is assigned directly to the decal node. This way Godot keeps control on how the atlases are done under the hood (which most likely will create a more optimized sytem). Thoughts?

betalars commented 1 year ago

If we allow using ViewportTexture, I don't think this needs to be an option within Light3D or Decal. We should just document the performance caveats of using a ViewportTexture that uses the Always update mode with decals or light projectors.

But then explain to me ... if I am not allowed to use a dynamic texture, how am I supposed to do a projector using the projector property?

I mean projectors do exist in some games and I would assume that a projector property would allow me to implement a projector.

clayjohn commented 1 year ago

@betalars I think you misunderstand what Calinou is saying. Calinou is saying that there shouldn't be a setting on the Light/Decal to enable dynamic updates, it should just work automatically.

betalars commented 1 year ago

@betalars I think you misunderstand what Calinou is saying.

hm okay I see your point, sorry about that ...

golddotasksquestions commented 1 year ago

I just tried to add a ViewportTexture as SpotLight3D light_projector in Godot 4.2 dev6 and everything seemed to work well in the editor. When running the scene it looked like there was no light_projectortexture assigned at all.

This very much feels like a bug to me. I think if it's possible in the Editor viewport, it should be possible when running the scene as well. If there are limitations (like light_projector can't be updated), then they need to be documented. But having this divergence between editor and running scene seems like the worst of all options to me.

For the record: I very much would like to use ViewportTextures as textures for both Decals and SpotLights.

so1975 commented 1 week ago

Minimal reproduction project

viewport-texture-decal-issue.zip

I tested this(4.3), and there was no need to await (in this case). If textures are set null and refreshed %SubViewport.get_texture(). But I see the point, texture is not updated if not resetted.

func _update_inner():
    max_health = max(max_health, 0.0)
    health = clamp(health, 0.0, max_health)
    if not is_inside_tree():
        return
    %Decal.texture_emission = null
    %Decal.texture_albedo = null
    %Decal.size = Vector3(radius * 1.75, 0.6, radius * 1.75) * 2.0

    %RingMesh.material_override.set_shader_parameter(&'max_health', max_health)
    %RingMesh.material_override.set_shader_parameter(&'health', health)
    %RingMesh.material_override.set_shader_parameter(&'radius', radius)

    %Decal.texture_albedo = %SubViewport.get_texture()
    %Decal.texture_emission = %SubViewport.get_texture()
    %SubViewport.render_target_update_mode = SubViewport.UPDATE_ONCE