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 for multimesh motion vector corruption #95270

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Lssikkes
Copy link
Contributor

@Lssikkes Lssikkes commented Aug 8, 2024

Multimeshes can get corrupted when using multimesh_set_buffer, and subsequently using the individual set API afterwards. This commit fixes the issue by making sure motion vectors are always disabled after resetting the instance count, and filling both halves of the buffer on initial multimesh_set_buffer motion vector upgrade.

@Lssikkes Lssikkes requested a review from a team as a code owner August 8, 2024 06:55
@akien-mga akien-mga added this to the 4.4 milestone Aug 8, 2024
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

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

Change looks good to me and makes sense. I haven't personally tested it but I discussed the change with @Lssikkes and we agreed this seemed like a good way to fix the issue.

I'll wait for someone else to verify and make sure it doesn't cause regressions. This could make it either for 4.3 or wait to be cherry-picked for 4.3.1.

@Lssikkes Lssikkes force-pushed the ls-multimesh-reset-only branch from 94d2146 to d6eaa61 Compare August 8, 2024 14:10
@Lssikkes Lssikkes force-pushed the ls-multimesh-reset-only branch from 515b65c to 52cd5ac Compare August 8, 2024 14:19
@Lssikkes
Copy link
Contributor Author

Lssikkes commented Aug 8, 2024

Thank you AThousandShips & Dario. Made the style changes.

@clayjohn clayjohn added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 9, 2024
@ciso507
Copy link

ciso507 commented Aug 16, 2024

This is going on on godot 4.3 stable Compiled version"
i had an issue with c++ multimesh, it doesnt update the visuals of motion bullets.. but it keeps updating the position.
Im also using c++ multimeshes for decorations ... static enviromental elements>> it updates the visuals and also the position.

@akien-mga
Copy link
Member

@ciso507 This PR hasn't been merged yet, so it's expected that you might still see this bug in 4.3.stable.
It will be merged soon (like in minutes, I'm working on a first merge batch) for 4.4, and cherry-picked for 4.3.1.

@akien-mga akien-mga merged commit b1c624b into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@ciso507
Copy link

ciso507 commented Aug 17, 2024

@ciso507 This PR hasn't been merged yet, so it's expected that you might still see this bug in 4.3.stable. It will be merged soon (like in minutes, I'm working on a first merge batch) for 4.4, and cherry-picked for 4.3.1.

Case#1:
I tried to reproduce on a brand new project and the bullets visuals will show if i dont animate the sprite trough the shader, if its just without animation it will show . It happens when i turn on HDR. without HDR it animates and show each individual mesh from the multimesh2d without issues.

Case#2: (I created a new project for this and i couldn't reproduce it)
And there is this error for normal instantiate of a bullet scene : error "" Failed to instantiate scene state of "", node count is 0. Make sure the PackedScene resource is valid. "" I also noted that my biggest scene loaded like 30% faster nice job 👍

@Lssikkes
Copy link
Contributor Author

Lssikkes commented Aug 19, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

Awesome, thank you so much :)

I think there are some issues remaining, I saw one case where just custom data/color updates could still cause confusion with the buffers, and the colors would start to flutter between state 1 and 2. I think I'll make a test scene soon to try to test all these different permutations of being able to use MultiMesh, and see which combination of things are still causing issues. This MR already improves things by quite a bit though.

Case#1:
I tried to reproduce on a brand new project and the bullets visuals will show if i dont animate the sprite trough the shader, if > its just without animation it will show . It happens when i turn on HDR. without HDR it animates and show each individual ?
mesh from the multimesh2d without issues.

This might be a separate issue - is 2D using motion vectors? Fullscreen motion blur?

@Lssikkes
Copy link
Contributor Author

jitter_bug
(the road is rendered through custom data with a shader interpreting them as texture coordinates)

@Lssikkes Lssikkes deleted the ls-multimesh-reset-only branch August 19, 2024 08:47
@ciso507
Copy link

ciso507 commented Aug 20, 2024

@Lssikkes @akien-mga
Thanks for the reply. I haven't interacted with motion blur for 2d; i dont know if its triggered by default (where can i check?) >> this shader works without hdr it will animate the texture if the multimesh instance2d that has a bool is enabled [should animate: true] now if i turn the enabled while hdr is enabled it will not display the texture or the animation... and the meshinstance2d shows for 0.2seconds and then it dissapear.
The shader im using :

<
shader_type canvas_item;

uniform float col;
uniform float row;
uniform vec4 custom;
uniform vec4 glow_color;
uniform float glow_intensity : hint_range(0.1, 47.0); // New uniform for glow intensity

void fragment() {
    if (custom.r == 1.0) {
        vec2 uv = UV;

        if (col != 0.0 || row != 0.0) {
            uv = vec2(uv.x / row, uv.y / col);
            uv.y += custom.b / col;
            uv.x += custom.g / row;
        }

        // Sample the texture
        vec4 texture_color = texture(TEXTURE, uv);
        
        // Only apply glow if the texture has opacity
        if (texture_color.a > 0.0) {
            // Apply glow to the texture, ensuring it is masked by the alpha channel
            vec4 glow = texture(TEXTURE, uv) * glow_color * glow_intensity * texture_color.a;

            // Mix the original texture and glow based on the alpha channel
            COLOR = mix(texture_color, glow, texture_color.a);
        } else {
            COLOR = texture_color;
        }
    } else {
        COLOR = vec4(0.0, 0.0, 0.0, 0.0);
    }
}
>

hdrErrorGodot

booleanAnimation

@ciso507
Copy link

ciso507 commented Aug 27, 2024

@akien-mga hey it seems that it gets fixed if i change the timer.wait_time ... that sends the signal for the width and height to change the frames, well this doesnt make sense to me that it gets fixed like this but its fixed.

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

6 participants