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

Remove SkeletonModificationStack3D, and Skeleton3D api cleanup #71137

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Jan 10, 2023

Removes all 3D modification resources.

SkeletonIK3D is a node and still supported.
Remove deprecated Skeleton3D functionality for 4.0, so we can add it back in 4.x.
Remove local_pose_override feature from Skeleton3D and BoneAttachment3D.
Expose Skeleton3D::get_version() so IK scripts/extensions can cache bones.

Note: This change only affects 3D. SkeletonModification2D will work as before.

cc @akien-mga @TokageItLab @fire @TwistedTwigleg

@lyuma lyuma added this to the 4.0 milestone Jan 10, 2023
@fire
Copy link
Member

fire commented Jan 10, 2023

I support this, of course the ci has to pass.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 10, 2023

I remember that SkeletonModification2D needed to be ported as a Node. Are you working on that as another PR?

@lyuma lyuma marked this pull request as ready for review January 10, 2023 11:07
@lyuma lyuma requested review from a team as code owners January 10, 2023 11:07
@lyuma lyuma force-pushed the remove_modification_stack_3d branch from 0e4480f to cb4749d Compare January 10, 2023 11:07
@lyuma
Copy link
Contributor Author

lyuma commented Jan 10, 2023

@TokageItLab No, I do not think SkeletonModification2D makes sense as a node. This is why I marked those APIs Experimental, while 3D were marked Deprecated.

Skeleton2D works fundamentally differently from Skeleton3D, in that in 2D the bones (the data model) are nodes.
The Skeleton2D node acts more as a driver for the bones, and so I found it relatively intuitive to work with the modification system, since that is one of the only purposes of the skeleton node.

Furthermore, the implementations of SkeletonModificaiton2D have been tested (at least at end of alpha) and are working. I see no reason to change the code and risk breaking them. My prior attempt to rewrite 2D modifications as nodes was full of bugs and would have required extensive debugging.

In contrast, I found Skeleton3D to be very difficult to work with, because it was acting as both the data model (bones) and the execution (modifications): also 3D skeletons tend to be far more complex... and on top of that, the 3D algorithms had broken due to various engine changes

As much as it would be nice for 2D and 3D to be analogous, for reasons unbeknownst to me, Skeleton2D and Skeleton3D were designed to work very differently from each other, so they require different solutions.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 10, 2023

I know that 2D and 3D Skeletons are different in their management of rests and poses, as well as Animation process.

In particular, what broke ModificationStack3D was the fact of changing the way of handling pose with rest in Godot 4 from 3, but I am not sure anymore.

However, there is no reason to make the Skeleton API so different between 2D and 3D, except for whether the Bone is a Node or a Property. For example, PhysicalBone1 also has 2D and 3D, both of which are Node.

In the first place, it was pointed out that the ModificationStack as a resource is out of favor with Godot's design. It should not have an API for IK within the Skeleton class. By separating it from the Skeleton by making it a Node, it may be easier to share settings among multiple Skeletons and prioritize them with animations.

At the very least, ModificationStack2D being a Resource is "not make sense" and "needs to be fixed". So, it should still keep being experimental and definitely needs to be ported as a Node in the future.

...For now, I agree that ModificationStack3D should be removed as RC, since it is no longer usable. And after first modifying ModificationStack3D, we need to modify ModificationStack2D as well in the future.

Footnotes

  1. By the way, I don't know why there is a discrepancy that PhysicalBone3D is not a child class of RigidBody3D; there are a large number of bugs in PhysicsBone as well, which will need to be fixed at some point.

@Zireael07
Copy link
Contributor

I'm confused. Will there be an IK solution in 4.0 (even if an imperfect one)?

@lyuma
Copy link
Contributor Author

lyuma commented Jan 10, 2023

I'm confused. Will there be an IK solution in 4.0 (even if an imperfect one)?

@Zireael07 , 4.0 will have SkeletonIK3D, same as the 3.x SkeletonIK. But the newly-written IK implementations are not ready yet. Removing the non-working implementation will leave us more room to finish developing good IK solutions and nodes for 4.1 and beyond. Also, there's a good chance some of the code you see deleted here will be added back in another form.

PhysicalBone3D
that is so strange. Why is it not a RigidBody3D?
As for PhysicalBone2D it actually behaves differently from 3D in that a skeleton modification is required for them to simulate 2D physics. Just another confusing difference.

I agree with exploring a new approach for 2D. I just don't want to remove a working system. Furthermore, in 2D, there are fewer bad APIs so there will be lower burden in maintaining a deprecated SkeletonModification2D together with anything we develop that replaces it, should that happen.

I'm willing to accept your opinions. This is all code we will have to maintain together.

@fire fire requested a review from a team January 10, 2023 16:18
@TokageItLab
Copy link
Member

TokageItLab commented Jan 11, 2023

If we keep SkeletonIK3D (I think we should remove it if ModificationStack3D porting is done), at least get_bone_axis_forward_vector() should be moved to the SkeletonIK3D from Skeleton3D.

Removes all 3D modification resources. SkeletonIK3D is a node and still supported.
Remove deprecated Skeleton3D functionality for 4.0, so we can add it back in 4.x.
Remove local_pose_override feature from Skeleton3D and BoneAttachment3D.
Expose Skeleton3D::get_version() so IK scripts/extensions can cache bones.

Note: This change only affects 3D. SkeletonModification2D will work as before.
@lyuma lyuma force-pushed the remove_modification_stack_3d branch from cb4749d to fd25bb5 Compare January 12, 2023 20:05
@reduz
Copy link
Member

reduz commented Jan 13, 2023

This looks good to me. My question is whether SkeletonIK node is working as intended at this point because I remember reports it wasn't.

@lyuma
Copy link
Contributor Author

lyuma commented Jan 13, 2023

Yes, I will make sure SkeletonIK3D works at least as well as the SkeletonIK node in 3.x.

I'll take a look at this issue first: #65167 since it's stretching bones in a way that shouldn't be possible, so it's probably an API issue. I suspect if something broke, it's likely caused by the change to make bone_pose include bone_rest

@reduz
Copy link
Member

reduz commented Jan 13, 2023

okay, looks good then

@akien-mga akien-mga merged commit 030a95d into godotengine:master Jan 13, 2023
@akien-mga
Copy link
Member

Thanks!

@Sarah-Duck
Copy link

Sarah-Duck commented Jan 15, 2023

We're currently using the CCDIK for our game, and it's working very well. The axis lock makes it very useful. Attempting to replace it with the SkeletonIK3D node results in unusable results at the moment. It tends to twist the body of our characters in ways that the CCDIK does not, and as you can see the head is rotated very strangely (I suppose the solution would be to curate the magnet tip in code). Will there be no alternative for 4.0? It would be nice to have some way to use it, whatever form it takes.

ik.issues.mp4

@fire fire deleted the remove_modification_stack_3d branch January 15, 2023 23:51
@fire
Copy link
Member

fire commented Jan 16, 2023

I am working on an ik solver for 4.x but it's not ready at this moment.

Here's a youtube video.

Watch the video

@jitspoe
Copy link
Contributor

jitspoe commented Jan 19, 2023

So I just synced and now functions like set_bone_local_pose_override are gone. What's the new way to do this? I have a cylinder rotating on another cylinder that rotates, as seen here:

https://twitter.com/jitspoe/status/1558491739513569281

Edit: set_bone_pose_rotation() seems to work for my case.

@shuvit-game
Copy link

make sure SkeletonIK3D works at least as well as the SkeletonIK node in 3.x.

Is there a PR or anywhere to track progress on this?

@realkotob
Copy link
Contributor

@shuvit-game I believe it is this one #70887

@shuvit-game
Copy link

That is for 4.x. I am concerned with tracking the regressions in 4.0 now that all of the reported issues have been closed. Are they release blockers as indicated above?

@realkotob
Copy link
Contributor

@shuvit-game Blockers for 4.0 are indicated on the Projects page, along with Regressions, Crashes, etc.

https://github.com/orgs/godotengine/projects/28

@shuvit-game
Copy link

@reduz asked if it was working as intended. It is not and @lyuma committed to working on it. I'm asking if there is any documentation on the work specifically mentioned here: #comment-1382261401 It was a very vague and ambiguous exchange.

@realkotob
Copy link
Contributor

realkotob commented Jan 22, 2023

@shuvit-game Thank you for clarifying, I don't think I've seen such a PR, and lyuma does not have any recent public PRs after that. Perhaps someone else can point to an existing tracking page or make one.

@Noiros
Copy link

Noiros commented Jan 25, 2023

Hello, it would not be possible to leave the modifictionstack and to add the old ik, it will ask me a huge work to carry my character... without being able to have the same result as before

@YuriSizov
Copy link
Contributor

@Noiros No, it won't be possible to do that. This solution is no longer supported and a proper system will be added in a future 4.x release. Leaving it in the engine until then would require us to break compatibility in a minor release, which is a bad experience for everyone, yourself included.

I'm sorry that you'll have to rework your project, and that at this point the same level of quality and fidelity is impossible, but this is why working with a pre-release version of a tool is risky and is not encouraged if you are not prepared for something like this to happen. We're being very clear about that.

@lyuma
Copy link
Contributor Author

lyuma commented Jan 26, 2023

@Noiros To be clear, I intend to get the classic SkeletonIK3D node (same as Godot 3.x) up and running in time for 4.0 but upgraded to use the CCDIK algorithm from the now-removed modification stack instead of FABRIK. (skeletonIK3D currently has bugs, so that's what I will be fixing)

that's the best I can do in time for 4.0. As Yuri said, we already lost the modifications, but there should be one working IK solution in place for 4.0 even if it's not quite as flexible.

lyuma added a commit to lyuma/godot that referenced this pull request Jan 31, 2023
Also removes unused override_mode property since we removed the local pose override feature in godotengine#71137.

Fixes godotengine#72407
Streq pushed a commit to Streq/godot that referenced this pull request Feb 9, 2023
Also removes unused override_mode property since we removed the local pose override feature in godotengine#71137.

Fixes godotengine#72407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.