drwhut / tabletop-club

An open-source platform for playing tabletop games in a physics-based 3D environment for Windows, macOS, and Linux! Made with the Godot Engine.
https://tabletopclub.net
MIT License
1.27k stars 55 forks source link

Bug: Rotated cards flipping problem #133

Closed elmodor closed 1 year ago

elmodor commented 1 year ago

Describe the bug Rotated cards look weird when flipped. It seems they try to flip on the world axis instead of it's current orientation. Also, if you continue to press the F key rapidly, it gets completely messed up. Probably because of the incorrect flipping described above.

To Reproduce Steps to reproduce the behavior:

  1. Rotate card
  2. Flip it
  3. (Flip it rapidly)

Expected behavior The card should flip around it's own axis, a normal flip to the right

Screenshots rotate_1.webm rotate_2.webm

Version v0.1.0 Beta 1

drwhut commented 1 year ago

The flip when rotated I think is expected, since it still stays in the correct orientation, but the sides have been flipped. However, the rotation changing completely when flipping it rapidly is definetly a bug.

elmodor commented 1 year ago

They do flip yes, but the animation looks weird. I still think this is a bug - the rotation seems to be done around the wrong axis / in the wrong coordinate system. If the card would flip the same as it does with the default orientation, then there would be no issue with flipping rapidly (since this issue is also only on rotated cards).

They should flip like this (I rotated the camera): like_this.webm

Not like this (I rotated the object): not_this.webm

You can see that in the second video the card makes two rotations - one back to the original orientation and the flipping simultaneously (and back).

elmodor commented 1 year ago

So if I didn't took a wrong turn, I narrowed down the problem to Pieced.gd func _apply_hover_to_state angular_velocity calculation

The hover state is applied to reach the target state from the current state. However, the angular_velocity is calculated from the difference of the current and target state. I think this is the issue. When an rotated objected is flipped, the angular_velocity is != 0 in all 3 axis. This is why the flipping occurs differently even on the same rotation.

What should be done: In case of rotation we want to only modify the Y axis. In case of flipping we want to only modify the Z axis.

Therefore the Piece would need to know if it is currently being rotated or flipped to apply the velocity to the correct axis.

Does that sound right?

drwhut commented 1 year ago

The way I programmed that function to work was to account for all possible "rotations", and ideally I would like that to still be the case, without using any outside variables. Maybe there's a way to modify the rotation so it's "nicer" within the function by checking the current and target rotation?

By the way, I'll put the relevant code here for convenience:

        # Force the piece to the given basis.
    var current_basis = state.transform.basis.orthonormalized()
    var target_basis = hover_basis.orthonormalized()
    var rotation_basis = target_basis * current_basis.inverse()
    var rotation_euler = rotation_basis.get_euler()
    state.angular_velocity = ANGULAR_FORCE_SCALAR * rotation_euler
elmodor commented 1 year ago

I don't think this would work.

Let's take a look at two rotations:

Based on these two values only, how do you want to decided in which direction you want to rotate? Positive direction, negative direction? Which axis? You can't. You will take the shortest way. That is what is happening currently - that's why the flip animation is rotating around different axis based on how the object is currently rotated.

So theoretically you don't want to take the shortest rotation - you want to rotate around a specific axis only. This information cannot be obtained from what the function currently knows.

You can keep the function for normal rotations. But in case of a flip you will need a specific function to handle this animation/rotation which knows that it should only rotate in the positive Z direction. Or extending the current one with this information. The flip should always be in the same direction, around the same axis.

At least I don't see how this would work if we only know these 2 rotations. Maybe someone can enlighten me :smile:

Also we should maybe use quaternions to avoid the euler shortcomings.

drwhut commented 1 year ago

Surely we can tell if the object has been flipped by looking at the rotations? Flipping the object rotates it by 180 degrees in the Z (back/front) axis, so that means both the X (left/right) and Y (up/down) axis flip to the other side. If we can check that's what happened, then surely we can at least get the rotation started in the right direction?

With regards to the quaternions, I do agree, but it'll take some time to switch from using Basis to Quat for that system - I'll probably get started on that today, as it would also optimise bandwidth usage.

elmodor commented 1 year ago

Sure you can do that, but wouldn't that just be a hacky workaround? Guessing if this is a flip? If you try such a guess work, why not use a variable to indicate that this is a flip - get it started in the right direction and then you can set back the flip variable to false. More secure, less guess work. Would it be that bad to have an outside variable for this? Of course guessing should work but I think it would be safer and - most of all - cleaner to use a variable indicating a flip has started?

Maybe the quaternion will actually already fix something here. I'm still not sure why this jittering happens sometimes when flipping. jitter.webm

drwhut commented 1 year ago

Personally I think it's better if we keep the _apply_hover_to_state function as generic as possible, and only let it interface with the current physics state. The main issue I have with adding another variable is that the variable itself is inheriently based on the physics state (how else would we know the object has stopped flipping?) - so my argument is that we can cut out the middle man, and avoid future issues cropping up at the same time.

I do agree Quaternions in this situation sound promising, and they also have several functions for lerping to another rotation (which may solve the root problem we have).

drwhut commented 1 year ago

Okay, I managed to fix the issue using Basis! It pretty much involves ditching the idea of modifying just the angular velocity (since it only takes in euler angles), and lerp-ing the transform's basis directly over time. This does mean however that the piece does not have any angular velocity at all when it is let go, unlike the linear velocity (which I do want to keep, it is very handy for e.g. rolling dice)

    # Force the piece to the given basis.
    var current_basis = state.transform.basis.orthonormalized()
    var target_basis = hover_basis.orthonormalized()
    var middle_basis = current_basis.slerp(target_basis, 0.5)
    state.transform.basis = middle_basis
    state.angular_velocity = Vector3.ZERO

I'll do some extra tweaking and optimisation before pushing the changes. I'll also make a separate issue for using quaternions, since I still think that should be done at some point.

Here are the results:

https://user-images.githubusercontent.com/13645454/205108503-955910cb-ede2-4509-aedd-880c9e18ea8e.mp4

elmodor commented 1 year ago

Looks good. I was afraid that the angular_velocity is the culprit but didn't know you are willing to ditch it :smile:

Glad that it's a very simple fix.

drwhut commented 1 year ago

I'm going to miss letting go of cards mid-way through flipping them and seeing them catapult into space :cry:

elmodor commented 1 year ago

Well while it is a cool thing to do, it's not so cool when you are actually trying to play a boardgame :smile: even less so when it goes off knocking over all your other boardgame pieces

The ~sky~ table is the limit