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

Keying the initial value of a shader parameter inserts Variant::Nil and the animation does not work as expected #87040

Open
nezvers opened this issue Jan 10, 2024 · 8 comments

Comments

@nezvers
Copy link

nezvers commented Jan 10, 2024

Tested versions

  • Reproduceable in: 4.3.dev, 4.2.1.stable, 4.2.stable (either totally doesn't animate or as discrete keyframe values),
  • Reproducible??? in 4.1.1.stable (something not correct with one of uniforms in reproduction)
  • Not reproducible in 4.1.stable

System information

Godot v4.3.dev1 - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 31.0.15.3734) - Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz (8 Threads)

Issue description

In production game we noticed that shader parameters are not animated inside a game but looked OK in editor. Instead of interpolating values are set to values saved in a material resource.

As I was creating a reproduction project it was totally off in editor where it acted as discrete values on a float uniform.

4.2 I noticed a warning about shader float value
scene/animation/animation_mixer.cpp:879 - AnimationMixer: 'test', Value Track: '.:material:shader_parameter/red' uses a non-numeric type as key value with UpdateMode.UPDATE_CONTINUOUS. This will not be blended correctly, so it is forced to UpdateMode.UPDATE_DISCRETE. (User)

4.1.1 it looked correct for uniform float alpha until I introduced uniform float red and its value wasn't animated.

Steps to reproduce

  • new project
  • add Sprite2D with AnimationPlayer
  • create a float uniform in shader (didn't think about colors and vectors)
  • animate those parameters with animation player
  • in 4.2.1 production project animated in editor but not in the game. But in reproduction project keyframes acted as Discrete.

Minimal reproduction project (MRP)

Animate Shader Parameters 4.1.zip
Animate Shader Parameters 4.1.1.zip
Animate Shader Param 4.2.zip
Animate SHader Bug 4.2.1.zip
Animate Shader Bug 4.3dev.zip

@clayjohn
Copy link
Member

Looks like the warning comes from #83030 originally (4.2)

I don't understand why having a StringName for a key would stop a value from being updated continuously as the underlying value is not a string, its just using a string to access a member CC @TokageItLab

@clayjohn clayjohn added this to the 4.3 milestone Jan 10, 2024
@TokageItLab
Copy link
Member

TokageItLab commented Jan 10, 2024

I remember there was an issue somewhere about a shader parameter outputting a Variant type that was incorrectly determined (I recall it was an issue regarding the shader parameter inspector, but I can't remember where it is), and I think this issue is related to that.

@clayjohn
Copy link
Member

@TokageItLab What is confusing me is that we are checking the "key" and not the "value". It seems like the issue is that the "key" is a string/variant, even though the value we are trying to use is a number.

@TokageItLab
Copy link
Member

@clayjohn I do not know where you are referring to. Can you cite a specific code?

Probably related issue about guessing the type of shader parameters:

@clayjohn
Copy link
Member

@TokageItLab Here we are forcing the track to non-continuous because the key value is a StringName (as far as I can tell):

if (!Animation::is_variant_interpolatable(track_value->init_value)) {
WARN_PRINT_ONCE_ED("AnimationMixer: '" + String(E) + "', Value Track: '" + String(path) + "' uses a non-numeric type as key value with UpdateMode.UPDATE_CONTINUOUS. This will not be blended correctly, so it is forced to UpdateMode.UPDATE_DISCRETE.");
track_value->is_continuous = false;
skip_update_mode_warning = true;
}

The user says they are using a float value, but they are still running into the warning because the check appears to look at the key instead of the value (which is a float)

@TokageItLab
Copy link
Member

TokageItLab commented Jan 10, 2024

Are you getting a StringName in is_variant_interpolatable()? I assume that init_value is NIL, based on #75125

@TokageItLab
Copy link
Member

TokageItLab commented Jan 11, 2024

As expected, the problem was that the exported shader parameter was Nil if it is initial value.

image

As noted in #75125 (comment), as long as it is an initial value, the Variant type is not properly assigned, so keying to the initial value seems to insert Nil.

When the value is changed in AnimationKeyEdit, the proper type is assigned and it works fine.

image

This can be fixed by caching the Nil key in the AnimationMixer cache so that if another key has a type, it takes precedence over it, but this is a hack.

This is clearly a bug in the uniform parameter of shader language, as mentioned in the comments, and other problems are occurring elsewhere than in Animation, so the fix must be made on the shader language side.

@TokageItLab TokageItLab modified the milestones: 4.3, 4.x Jan 11, 2024
@TokageItLab TokageItLab changed the title Animating shader parameters doesn't work as expected Keying the initial value of a shader parameter inserts Variant::Nil and the animation does not work as expected Jan 11, 2024
@clayjohn
Copy link
Member

Thanks for taking a deeper look! I'll check out the shader side and see if we find a solution

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

No branches or pull requests

4 participants