t->value within animation_tree.cpp is always null for Packed Array Types if without RESET track.

[KnightNine]

Godot version

v4.1.beta.custom_build

System information

Windows 10

Issue description

I fixed #73342 for myself and anyone who cares, so I added support for arrays to Animation::blend_variant(), turns out there is another issue that was preventing me from using array types in Animation::blend_variant(). That being the value variable of t that is fetched from TrackCacheValue *t = static_cast<TrackCacheValue *>(track); (at line 1447 within animation_tree.cpp) is null for array types.

This t->value value also happens to be the first input param for blend_variant():

t->value = Animation::blend_variant(t->value, value, blend);

Meaning: since this variable is null, no blending can occur. And the PackedArray becomes empty when trying to use PackedArray animation tracks in animation tree node continuous_update_mode transitions, even when blending is supported within animation.cpp.

I later figured out that adding a RESET track with the same update mode and array size for each PackedArray being animated makes everything work, but I don't know why...

Without Reset Tracks (animated polygon arrays become empty and fail to animate as soon as the animation tree becomes active):

With Reset Tracks (it works):

Consider this a "future bug" for when this feature is implemented, there probably shouldn't be an instance where the RESET track is required as that just adds more unnecessary steps to animating PackedArrays. Also, anyone know why this happens?

Steps to reproduce

in this custom build, animation.cpp should damn well support PackedArray types for blend/interp/sub/add but it still results in the same issue seen in #73342 (steps to reproduce within)

Minimal reproduction project

N/A

[AThousandShips]

This is a bug report for code not in the codebase, anticipating an issue with one specific implementation of a fix for an issue, bugs should only be opened for issues that exist in the engine

And the issue is simply that the tree doesn't support this type, and therefore doesn't support it, so any change to add this would have to make it support it, right?

I fixed #73342 for myself and anyone who cares

Cares? There's an issue open for it and a related proposal, so people care, open a PR for your fix if you like, but this comment is really confusing

[KnightNine]

@AThousandShips

This is a bug report for code not in the codebase, anticipating an issue with one specific implementation of a fix for an issue, bugs should only be opened for issues that exist in the engine

It is for code that is "in the codebase" this issue stems from the fact that there is a function which expects a certain value but it only ever receives null unless the arbitrary prerequisite of a RESET track is met.

The changes made are relevant to the function and not the caller so I don't think the solution's specificity has any relevance here. Just wanted to point it out that there is something that is probably missing and I don't know why... maybe it's helpful, maybe it's not.

Cares? There's an issue open for it and a related proposal, so people care, open a PR for your fix if you like, but this comment is really confusing.

I don't know how to do the formatting yet. maybe I'll try sometime, but it would be very similar to a PR that already exists with one addition. Tbh, they should just add all the PackedArray support to the one PR.

[AThousandShips]

But you can't use thes tracks at the moment right? Without those changes? So this would have to be fixed as part of enabling that kind of functionality

Correct me if I'm wrong:
It is not possible currently to animate these kinds of tracks, and because of this this error cannot occur

Correct?

[KnightNine]

@AThousandShips

But you can't use thes tracks at the moment right? Without those changes? So this would have to be fixed as part of enabling that kind of functionality

Correct me if I'm wrong: It is not possible currently to animate these kinds of tracks, and because of this this error cannot occur

Correct?

You aren't wrong but I don't understand how it would be justified to ignore this issue if it is distinct and will most likely need to be addressed sometime in the future. I see the solution to this problem as pretty linear unless a bunch of the animation code is planned to be redesigned or something, idk..

You decide really. I'm not in charge of this repo so why are you confirming this with me? My opinion about this matters little here.

[AThousandShips]

It's not a bug, and it only will be one of the person who implements the relevant function wrong, you're essentially reporting your own attempted code as a bug, do you see?

Thank you for pointing out this need, though I assume anyone implementing this would have taken a look at the animation classes to fix this as well, but this is not a bug, bugs should only be reported for issues that exist in the engine, not ones that might appear. Understand? I'm trying to work out here the logic of making bug reports.

[KnightNine]

Nothing is wrong with my function changes, it handles its input parameters properly and now with additional support for PackedArray types.

I fail to understand how that would be my own oversight for not predicting that the functions would be called with invalid or missing parameters from somewhere else within the code (likely as it was never tested with PackedArrays)?

[AThousandShips]

But if you make changes to allow for new behavior you have to account for where that is used?

And you need to anticipate how it is used, because how it "should" be used isn't relevant if that's not how it's actually used.

If you change some part of the engine then any other parts of the engine that use those parts need to be considered, things aren't isolated.

Again, how does it work currently with packed arrays? Does it allow them? Does it throw errors? Etc.

Bugs are things that happen with the engine as it is, not with changes to it

Obviously any change to the blending code should involve changes to account for this functionality in the animation system, that's how things are done.

[AThousandShips]

This is only a bug if someone causes it to be one, assuming what you said is the case that this does not happen currently without adding blending to the arrays.

This only happens if the person implementing support for blending packed arrays fails to account for this.

Thank you for bringing this to attention, and it should be accounted for if/when implementing blending for those types.

And in all honesty, this is a good learning experience, to realize that if you change something you can introduce regressions elsehwere and that's something you have to keep in mind when doing contributions

[KnightNine]

I get that it is an issue that would not be encountered had this functionality not have been implemented at all. but technically t->value is still null when it probably wasn't supposed to be, whether or not this functionality had been enabled or not.

I do still see it as a bug which exists within the engine whether or not I changed anything.

I fixed what I was able to fix. ideally I would know how to do it flawlessly and without help, but that isn't reality, that's why I'm here asking about this unexpected behavior.

Thank you for bringing this to attention, and it should be accounted for if/when implementing blending for those types.

👍 hopefully it is helpful. goodbye.

[AThousandShips]

I'd suggest you open a PR for your changes, because this is for the engine and contributing to it, alternatively use the commuy channels such as rocketchat

[TokageItLab]

Closed by #80813 (but this issue was mostly same with the #80971)