Details

Reviewers

Dalai Felinto (dfelinto)
Germano Cavalcante (mano-wii)

Maniphest Tasks

T99997: Walk mode: when you press space button too fast, gravity disables

Commits

rB18f0caa6c117: Fix T99997: Calling teleport without waiting for the previous event disables…
rB2a7b8e78e192: Fix T99997: Calling teleport without waiting for the previous event disables…
rBead3fc4a075d: Fix T99997: Calling teleport without waiting for the previous event disables…

Summary

During teleport event, gravity is disabled and WalkMethod
is stored in teleport.navigation_mode which is used later to reset
the status after execution. Calling teleport events consecutively
will change the initial WalkMethod value. So update it only on the
first call. Also remove else condition as it stops the previously running
teleport when the new teleport call fails to find a hit point.

Diff Detail

Repository: rB Blender

Event Timeline

Pratik Borhade (PratikPB2123) requested review of this revision.Jul 29 2022, 12:57 PM

Pratik Borhade (PratikPB2123) created this revision.

Pratik Borhade (PratikPB2123) edited the summary of this revision. (Show Details)

Pratik Borhade (PratikPB2123) added reviewers: Dalai Felinto (dfelinto), Germano Cavalcante (mano-wii).

Pratik Borhade (PratikPB2123) added a project: EEVEE & Viewport.

Pratik Borhade (PratikPB2123) added a task: T99997: Walk mode: when you press space button too fast, gravity disables.Jul 29 2022, 12:59 PM

See my suggestion. It needs to be tested though.

source/blender/editors/space_view3d/view3d_navigate_walk.c

856

I think it may make more sense to move this above (before the teleport->state = WALK_TELEPORT_STATE_ON) and do something like:

/** Only store the current navigation mode if we are not already teleporting,
  * otherwise we risk storing the temporary free mode instead.
  * This happens when teleporting is called while a current teleport is still happening. */
if (teleport->state != WALK_TELEPORT_STATE_ON) {
    teleport->navigation_mode = walk->navigation_mode;
}

This revision now requires changes to proceed.Jul 29 2022, 2:30 PM

Patch updated as per the reviewer's suggestion

I didn't think of this solution before. Works well so far

The new code block has to be inside the if (ret) { statement, no?

I don't think this is needed.
But I'll update the patch if this additional if check is expected inside the if (ret) { block.
Any particular reason BTW? :)
I see why this check is needed inside if(rect) { block. Ignore this comment.

Pratik Borhade (PratikPB2123) added a comment.Jul 29 2022, 4:28 PM

This comment was removed by Pratik Borhade (PratikPB2123).

Pratik Borhade (PratikPB2123) updated this revision to Diff 54186.Jul 29 2022, 5:49 PM

LGTM, it doesn't need to mention T99997 in the code to be honest.

This revision is now accepted and ready to land.Aug 1 2022, 9:27 AM

Remove report number from comment

Dalai Felinto (dfelinto) accepted this revision.Aug 1 2022, 2:43 PM

I tried the latest patch and if I teleport too frenetically (pressing space multiple times while falling) I still see gravity being turned off after awhile.

So I'm not sure if this is fixing the issue.

This revision now requires changes to proceed.Aug 1 2022, 5:18 PM

@Dalai Felinto (dfelinto) hi, I'm not really able to replicate this with present patch. I might loose my spacebar key :p
Are you able to replicate same issue with initial patch also?
.blend file that I'm using.

walk-gravity.blend815 KBDownload

It looks better, but I can confirm that it hasn't been fully fixed:

Apparently the problem still happens when, while teleporting, we choose an empty area of the screen to teleport again.

Apparently the problem still happens when you, while teleporting, choose an empty area of the screen to teleport again.

Thanks, I can redo this case :D
I'll update the patch shortly

Remove else condition as it stops the previously running
teleport when the new teleport call fails to find a hit point

Pratik Borhade (PratikPB2123) marked an inline comment as done.Oct 6 2022, 2:06 PM

Pratik Borhade (PratikPB2123) edited the summary of this revision. (Show Details)Oct 6 2022, 2:34 PM

If I understood the last change correctly, now if we point to a region without a hit, the teleport state is no longer canceled (so we don't have to worry about other states that need to be restored).

Sounds good to me, especially since the teleport duration is pretty quick.

(But personally, if we have to change the behavior, maybe WALK_MODAL_TELEPORT should work with toggle. If the state is ON it is canceled or vice versa. But it's not something to worry about in a bug fix though).

If I understood the last change correctly, now if we point to a region without a hit, the teleport state is no longer canceled

Right. And continue teleporting to the previous "valid" hit (I mean follow the last teleport operation in which hit is detected).

On second thought, I think it's good to avoid any functional changes in the operator. The teleport time is configurable and some users may already be used to how to stop the teleport.

@Germano Cavalcante (mano-wii), so you agree with the current diff, right? :)

The current diff changes the behavior of the operator (which is perhaps best left unchanged), but it's a small change so if it's preferred that way that's fine with me.

ok, I'm fine with either. We'll wait for @Dalai Felinto (dfelinto)'s reply :)