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

Mouseover actions for flipping and reset orientation #154

Closed elmodor closed 1 year ago

elmodor commented 1 year ago

Fixes/Solves So I thought I give mouse overs a try. Having cards on the ground and needing to flip them is a common use-case for the games I know.

Fixes #112 (except rotations, which are missing a keybinding all together so far - done in a new PR)

Well, the problem is: I can't flip (or rotate) the object on the ground. I have to lift it up - and afterwards drop it down. Lifting it up and flipping/rotating is easy enough. But how do I get it down again? I'm really not sure how to do this properly in Godot. I found a way to set a timer which triggers the stop_hovering function after a few ms but is this the way to go?

It works tho: flipping_on_ground.webm

Missing:

Is using timers a bad idea here? What could be used otherwise?

drwhut commented 1 year ago

Hmm... I'm not sure about using the hover system to flip the pieces when they're not already hovering.

While theoretically it would work, I'm concerned about using timers here as well - in an ideal world we shouldn't need to expect something to happen in a certain amount of time (which can sometimes fail, e.g. in low frame rate environments), we should wait until it says it is done.

I would personally use the Piece.request_set_transform method. While it doesn't have an animation, it is a method that has been tested numerous times, and it doesn't require the piece to be hovered.

elmodor commented 1 year ago

I really don't want to use Piece.request_set_transform. The flip and rotate should be animated. If we set the transformation directly it will just look "cheap". I'm sure this can be done with an animation properly. Even if it takes some time to implement correctly. I also added that the timer is stopped if the piece is picked up before the animation is finished -> so the hover is on the mouse.

Is there a possibility to force low frame rate to test this out? Apparently Timer should already consider delta for this specific case. So that would be cool if it could be tested.

What else could be used to perform an animated action?

drwhut commented 1 year ago

If you don't want to use request_set_transform, then I think a new system would need to be implemented and tested (and I'm not 100% sure if a new system like that would make it to the v0.1.0 release). Maybe it could be used for now as a temporary measure, then improved upon in a future release?

Is there a possibility to force low frame rate to test this out?

There is in the project settings, but you can only set integer values for the frame rate.

What else could be used to perform an animated action?

As of right now, nothing. Hence the new system I mentioned.

elmodor commented 1 year ago

There is in the project settings, but you can only set integer values for the frame rate.

So I tested this with 1 FPS and it still worked without an issue. It seems the Timer really takes the FPS into account.

If you don't want the Timer then I will change it to use transform instead as a workaround. Then we can create an issue to implement a proper system. When I said I don't want to use transform I was talking about as a final solution.

So let me know if I should change it to transform. I would change the code in Pieces.gd

drwhut commented 1 year ago

Yeah, that's fine. It's just I would rather use a simple, rigorously-tested system to implement the feature at this stage. Besides, there are a lot of places in the game where there could be animations, but aren't, just purely because I've prioritised stability in the early stages of development.

elmodor commented 1 year ago

Okay I removed the animations. But I'm not sure why you said request_set_transform would be working properly. It's working on the server but on the client the rotation and flipping is animated? And I have no idea why. It should set the transformation directly without applying any angular force?

EDIT: I think the problem now is that func _integrate_forces(state): for the client is using slerp to apply the transform rotation: var lerp_quat = client_quat.slerp(server_quat, TRANSFORM_LERP_ALPHA).normalized() while the server sets the transform directly.

Just keep it like this? And fix it properly once we will make an animation for this action?

drwhut commented 1 year ago

Ah, yeah that's because the server sends updates to the clients at a variable rate, so the client smoothly goes towards whatever the last server state was. Maybe setting _last_server_state_invalid = true in set_transform will fix it, but if it doesn't, don't worry about it too much.

elmodor commented 1 year ago

It only fixes it if the server is the one initiating the action. If the client initiates it, it will still use the slerp on the client because the server state is still the same on the client.

elmodor commented 1 year ago

Ready for review.

elmodor commented 1 year ago

Alright, let me know what you think about this version :)

I added the function is_piece_allowed_modify because the check would be duplicated quite often. Better avoid the code duplication there.

elmodor commented 1 year ago

To reproduce:

I think what happens is:

I suspect this happens because _original_shape_scales_saved is never set back to false

elmodor commented 1 year ago

Please take a look if this is the correct fix at the correct location. Should _original_shape_scales_saved ever be set back to false?

drwhut commented 1 year ago

No, it's a flag that should be set to true when the contents of _original_shape_scales has been set, as in to say "use these instead".

elmodor commented 1 year ago

The only observation I made was that, for whatever reason, the scale seems to start to drift very very slowly. In my eyes this seems like a floating point operation issue.

Hence I checked the difference between the old and new scale against an epsilon value - only if there's a difference then scale the object.

elmodor commented 1 year ago

This issue would not be the case if I would have used the hover basis :smile:

Additionally, when you flip or reset the orientation of any complex piece, the scale is being reset. You can try setting the Y scale of a pawn to 2, flip it -> scale resets.

The issue I found is that when setting the transform the scale is being lost. transform.basis does not have any scale because the scale is stored inside the collision shape. So it is obtained from get_current_scale. So I could do:

    var flipped_rotation = transform.basis.rotated(transform.basis.z, PI)
    flipped_rotation = flipped_rotation.scaled(get_current_scale())
    request_set_transform(Transform(flipped_rotation, transform.origin))

But even then the scale starts to drift at some point. Does the scale of the collision shape changes when the object is rotated and falls down? Is this still a floating point issue? Also the scale in x y z changes if you rotate around Y by PI based on the current orientation? At least:

# get_current_scale() is (8.6494 8.6494 1)
var flipped_rotation = transform.basis.rotated(transform.basis.z, PI)
flipped_rotation = flipped_rotation.scaled(get_current_scale())
# reset_orientation get_scale is (7.397342, 4.590821, 1.328522)
# doesn't matter if I scale first and then rotate, or rotate then scale. Same issue.

I tried to implement more floating point checks (e.g. in get_current_scale etc) but the issue is still persistent. So I'm not quite sure how to continue here.

elmodor commented 1 year ago

I assume the transform.basis issue is also responsible that the transform in the context menu is reset to 1 if you changed it, closed the context menu, open it again.

drwhut commented 1 year ago

Yeah, request_set_transform needs the scale inside the basis. The crux of the problem I think is that the scale of the collision shapes is independent of the rotation of the rigid body.

What I would try is starting off with the identity basis, scaling it with get_current_scale(), since the function does not take the current rotation into account, then rotating it with the current rigid body basis. Off the top of my head, it would be something like:

var scaled_basis = Basis.IDENTITY.scaled(get_current_scale())
var rotated_basis = transform.basis * scaled_basis

This is in fact what the camera controller does when setting the transform:

func _on_ApplyTransformButton_pressed():
    _hide_context_menu()
    var origin = Vector3(_transform_piece_pos_x.value,
            _transform_piece_pos_y.value, _transform_piece_pos_z.value)
    var angles = Vector3(deg2rad(_transform_piece_rot_x.value),
            deg2rad(_transform_piece_rot_y.value),
            deg2rad(_transform_piece_rot_z.value))
    var new_scale = Vector3(_transform_piece_sca_x.value,
            _transform_piece_sca_y.value, _transform_piece_sca_z.value)

    var new_basis = Basis(angles) * Basis.IDENTITY.scaled(new_scale)
    var new_transform = Transform(new_basis, origin)

    for piece in _selected_pieces:
        piece.rpc_id(1, "request_set_transform", new_transform)

If that does not work, let me know.

elmodor commented 1 year ago

I did that, but only for the rotation not for the flipping. I thought I could do the scaling directly on the transform.basis but it seems to be working if I start with IDENTITY. I think this should work. I will test a bit further and then update it.

Thanks!

elmodor commented 1 year ago

Alright, I was not able to break it with the new version.

I'm not sure but in general I don't really think this is intuitive that you always have to do this when calling set_transform. Maybe the function should be split to: set_rotation and set_translation and set_scale? And then the function can do this automatically because you know which part of basis changed. Is there a case where you want to apply both, rotation and translation? Currently you need to apply the scale everywhere before calling the function. If you agree then this should be done separately in a different PR.