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

Fix background bleeding through #1839

Merged

Conversation

Pheubel
Copy link
Contributor

@Pheubel Pheubel commented Oct 28, 2023

This resolves #1831

In order to fix this issue i implemented it as described in #1826 (comment)

I had to change a few things:

  • The structure of the default default dialog node scene has changed a little bit, I added two nodes in the background holder, a node that contains all the backgrounds that get swapped in. And a color rect called Overlay that uses the backgrounds to draw the actual background that needs to be displayed.
  • The default background scene has changed a little bit, instead of a color rect with a texture rect child, i have a parent SubViewportContainer with a viewport as child. The overlay relies on the viewport to get a texture of what is happening in the scene to the overlay. The texture provided by the viewport is actually dynamic, so any changes that happen inside the viewport do get drawn, so animated scenes as backgrounds should work just fine.
  • The code for the background submodule has been changed a bit, the role of _fade_in(float) and _fade_out(float) have been reduced to notifications that the background scene is fading in/out, i think this is the best fit for them, as the transition.
  • I added a whipe transition shader and material, this is what powers the overlay, it transitions between the previous and next background. For now, the feather and whipe texture have been set in the material, to 1 and a midtone gray respectively. This paves the road for possible custom transitions using whipe textures.

Breaking changes:

  • existing custom background scenes will most certainly break, as they do not return a ViewportTexture. In order to fix them, their scene structure has to be changed to have a SubViewportContainer as root with a SubViewPort as child, they can then put everything they had as child of the SubViewPort. Afterwards they need to make sure that the node references are all wired up correctly.
  • Custom transitions in _fade_in(float) and _fade_out(float) will likely behave in an unexpected way, i didn't have a use case to test how they would behave.

What this allows for in the future:

  • Custom transitions using self made textures (easy for developers) - The mechansim implemented for the transitions here makes use of a whipe texture with a feather already, the code needs to be altered in such a way that it keeps the current settings as a default if no whipe texture is provided. There might also need to be an option to indicate how the whipe texture should be applied (toggle between stretch to rect VS scaled to keep the aspect ratio) I don't think it would be a hard task, but requires a bit of UI design.

  • Custom transitions using custom shaders (harder for developers) - The shader would have to have a material replace the material in the overlay material, this would require a bit more thought, it will essentially be like the texture transitions, but will require some more code to make sure that the properties are set right. It will require a bit more UI work in order to get this to work well. I would imagine that if this were to be added, it will need to make use of the Key Value pair element that i made for Allow for signal events to optionally pass dictionaries instead of strings #1829, but more advanced to allow for data types outside strings. I think this feature is possible, but definitely an advanced feature.

@Jowan-Spooner Jowan-Spooner added Bug 🐞 Something isn't working Feature✨ Needs testing We don't understand this problem fully yet labels Nov 1, 2023
Pheubel and others added 3 commits November 6, 2023 17:44
This keeps all the new viewport and shader stuff but offloads most of the work to behind the scenes. This means background scenes can work just as they did previously. However they are manually put into subvieports+subviewportcontainers. The backgrounds-holder is now also the background-displayer.

_fade_out() and _fade_in() have been renamed to _custom_fade_out() and _custom_fade_in(). Right now they work "additionally" to the default fade which happens as well. If you know what you are doing you can work around this though.
I have to put further thought into custom transitions as I have no real idea what the best user-experience/use case might be.
@Jowan-Spooner
Copy link
Collaborator

Note to future visitors: The implementation of this has changed quite a bit.

@Jowan-Spooner Jowan-Spooner merged commit a7f565c into dialogic-godot:main Nov 10, 2023
@Jowan-Spooner
Copy link
Collaborator

@Pheubel Thanks for all the work and patience!

Invertex pushed a commit to Invertex/dialogic that referenced this pull request Jan 26, 2024
…dialogic-godot#1839)

This keeps all the new viewport and shader stuff but offloads most of the work to behind the scenes. This means background scenes can work just as they did previously. However they are manually put into subvieports+subviewportcontainers. The backgrounds-holder is now also the background-displayer.

_fade_out() and _fade_in() have been renamed to _custom_fade_out() and _custom_fade_in(). Right now they work "additionally" to the default fade which happens as well. If you know what you are doing you can work around this though.
I have to put further thought into custom transitions as I have no real idea what the best user-experience/use case might be.

---------

Co-authored-by: Jowan-Spooner <raban-loeffler@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐞 Something isn't working Feature✨ Needs testing We don't understand this problem fully yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event Background: Transitions with fade time cause scene background to bleed in
2 participants