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

Revise Teleport Target #7531

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Conversation

kphoenix137
Copy link
Collaborator

@kphoenix137 kphoenix137 commented Nov 11, 2024

Currently, if the player holds right click with Teleport selected, this causes the game to select a tile as the Teleport target, and then select the same tile as the queued Teleport target, resulting in the player casting every other Teleport with no movement.

This PR does the following:

If the player is casting Teleport and the new target tile is the same as the currently targeted tile (or any nearby tile) by the currently executed Teleport spell, get the relative distance between the player and the new target tile, and add that to the currently executed tile, then Teleport the player there instead.

This allows the player to keep the mouse still and continuously queue Teleport to move in a direction. However, this does not work consistently if the player is moving the mouse around while doing this, since the player will end up zigzagging, as the queued tile can end up being different enough at this point from the executed tile. This isn't a new problem, but an existing potential annoyance due to spells targeting fixed positions rather than relative positions. The only way around that is to completely move Teleport to a relative target system, however doing so can end up being incredibly disruptive to gameplay and I don't believe it should be done.

This fix is effectively the same as simply not letting the game queue a Teleport if the queued tile is the same as the executed tile, however doing that end up adding 1 frame to every cast animation, since the chained Teleports are no longer actually chained, but fresh casts. Instead, we preemptively get the new position to Teleport the player to, in order to maintain the queueing and not slow the player's casting down.

Before:

Desktop.2024.11.11.-.14.09.02.60.mp4

After:

Desktop.2024.11.11.-.14.15.20.61.mp4

@julealgon
Copy link
Contributor

@kphoenix137 after the player is teleported, are there no animation frames at all to still play?

I don't remember looking this up, but I assume actual teleportation happens on the hit-frame (around half-way through the casting animation) and then the rest of the casting animation plays on the target tile.

If that is indeed the case, you could instead only allow teleport to be queued after the hit-frame has already happened, in which case the new queued command will respect the new target tile based on the final destination + player cursor's new position.

I guess this would still potentially add that single frame of delay because of the standard animation-cancelling on hit-frame... oh well, if that's the case, disregard my suggestion.

This would work a lot more smoothly without animation cancelling 😛

@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Nov 11, 2024

@kphoenix137 after the player is teleported, are there no animation frames at all to still play?

I don't remember looking this up, but I assume actual teleportation happens on the hit-frame (around half-way through the casting animation) and then the rest of the casting animation plays on the target tile.

If that is indeed the case, you could instead only allow teleport to be queued after the hit-frame has already happened, in which case the new queued command will respect the new target tile based on the final destination + player cursor's new position.

I guess this would still potentially add that single frame of delay because of the standard animation-cancelling on hit-frame... oh well, if that's the case, disregard my suggestion.

This would work a lot more smoothly without animation cancelling 😛

Yes that would actually fall under what I described at the end. Doing so with a hacky fix would effectively be the same, except it would be extremely annoying for players who don't click and hold, as this means you wouldn't be able to queue teleport at all.

@StephenCWills
Copy link
Member

I don't think the 1-frame delay is all that important for a couple different reasons. However, I do feel that not being able to queue Teleport with a mouse-click during the previous casting animation would be a bit of a problem which almost entirely rules out any solution that would require a 1-frame delay.

Source/player.cpp Outdated Show resolved Hide resolved
Co-authored-by: Stephen C. Wills <[email protected]>
StephenCWills
StephenCWills previously approved these changes Nov 11, 2024
@qndel
Copy link
Member

qndel commented Nov 11, 2024

How about moving new code to where it's actually used? Mini optimization as currently it's being calculated even when you have no mana, moving it after that early return would handle that :D

@StephenCWills
Copy link
Member

I guess you could even put it inside the else, since that's the only branch that uses targetedTile.

@StephenCWills StephenCWills dismissed their stale review November 11, 2024 21:46

Qndel has a point

@qndel qndel merged commit 228aa7e into diasurgical:master Nov 12, 2024
23 checks passed
@kphoenix137 kphoenix137 deleted the revise-teleport-target branch November 12, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants