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

Fix bug in teleport anim #3684

Closed
wants to merge 4 commits into from
Closed

Conversation

newfrenchy83
Copy link
Contributor

Temp_unit.set_location necessary else pre and post teleport animation played in same place(arrival). It is unuse ro place 'don't scrolling 'if this bugs not fixed.

Temp_unit.set_location necessary else pre and post teleport animation played in same place(arrival). It is unuse ro place don't scrolling if this bugs not fixed.
@newfrenchy83 newfrenchy83 changed the title Revert don't sroll in teleport because bug in anim Fix bug in teleport anim Nov 3, 2018
@jostephd
Copy link
Member

jostephd commented Nov 3, 2018

Thanks for the ping @jyrkive.

@newfrenchy83 How do I reproduce the bug? Can you give an example of a unit that has a post teleport animation? I grepped but found none.

@jostephd jostephd added this to the 1.14.6 milestone Nov 3, 2018
@jostephd jostephd added UI User interface issues, including both back-end and front-end issues. Regression Issues that were not present in previous releases. labels Nov 3, 2018
@@ -87,6 +87,7 @@ void teleport_unit_between(const map_location& a, const map_location& b, unit& t
disp.scroll_to_tiles(a, b, game_display::ONSCREEN, true, 0.0, false);
else
disp.scroll_to_tile(a, game_display::ONSCREEN, true, false);
temp_unit.set_location(a);
unit_animator animator;
animator.add_animation(&temp_unit,"pre_teleport",a);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The location a is passed to add_animation(). Is adding set_location() the correct fix? Or should add_animation use the location passed to it in preference to temp_unit.get_location()?

Copy link
Member

@jostephd jostephd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. I confirm the bug and the fix (it happens with an ordinary Silver Mage when both source and destination are visible), but I have a question about the fix.

@jostephd jostephd added the Confirmed Issues that have been successfully reproduced by at least one developer. label Nov 3, 2018
@newfrenchy83
Copy link
Contributor Author

I had put set_location first in original location but when i use teleport with allow_vision=no, map scrolling in destination same if invisible. I moved this after scoll_to_tiles for scrolling was after teleport finished and don't have regression

@newfrenchy83
Copy link
Contributor Author

Unit animation use temp_unit, it must what location was edited before anim played else location was b (destnation) by default and not a fir pre teleport and b for pos teleport. This function was originally implemented before scrolling and will must be moved after and not cleared.

@@ -100,6 +101,7 @@ void teleport_unit_between(const map_location& a, const map_location& b, unit& t
disp.scroll_to_tiles(b, a, game_display::ONSCREEN, true, 0.0, false);
else
disp.scroll_to_tile(b, game_display::ONSCREEN, true, false);
temp_unit.set_location(b);
Copy link
Member

@jostephd jostephd Nov 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@newfrenchy83 Should this set_location call happen when b_visible is false, like it was before 2d34a75? I mean, should set_location be called both when b_visible is true and when it is false?

@newfrenchy83
Copy link
Contributor Author

Yes anmation called but if b not visible and oher side of map, map don't scrolling and player don't see post teleport anim

@newfrenchy83
Copy link
Contributor Author

Set_location must be called same if b invisible else post teleport played in a location

@jostephd
Copy link
Member

jostephd commented Nov 4, 2018

Set_location must be called same if b invisible else post teleport played in a location

Thanks. Could you push a fix please?

@newfrenchy83
Copy link
Contributor Author

i don't understood. what i must fux please.

@newfrenchy83
Copy link
Contributor Author

set_location called in both case b visible oe not, and pleced after uf visible for don't affect scrolling. i trusted already fixed regresdion problen ?

@newfrenchy83
Copy link
Contributor Author

When i use silver mage with allow _vision=no with fix map don't scroll in unit before end of teleport if b invisible. The scrolling after post teleport anim was because siver mage in my own side. A enemy in fog mustbbe can teleporting without scrolling.

@jostephd
Copy link
Member

jostephd commented Nov 4, 2018

set_location called in both case b visible oe not, and pleced after uf visible for don't affect scrolling. i trusted already fixed regresdion problen ?

Yes, I have tested and confirmed that the patch fixes the regression: when both a and b are visible, this patch causes the pre_teleport animation to be played at the correct hex.

Do we need to add else { temp_unit.set_location(...) }? (in addition to the existing patch)

When i use silver mage with allow _vision=no with fix map don't scroll in unit before end of teleport if b invisible. The scrolling after post teleport anim was because siver mage in my own side. A enemy in fog mustbbe can teleporting without scrolling.

Sorry, I don't understand. Are you saying that there is another bug, that involves a [tunnel] with allow_vision=no? And that moving the set_location from its original location to after the scroll_to_tiles call, fixes that bug?

Thanks for your patience.

@newfrenchy83
Copy link
Contributor Author

In fact i done an error set_location must be push in original location. It is not affect scrolling.

@newfrenchy83
Copy link
Contributor Author

I pushed set_location in right place, now and no bug.

@jostephd
Copy link
Member

jostephd commented Nov 5, 2018

Thanks. Looks good. 👍

I tested the three cases in #3578 as well as the "both source and destination" visible case and all work as expected. I'll merge this as-is if no other comments are received.

@newfrenchy83
Copy link
Contributor Author

Tkank

GregoryLundberg pushed a commit that referenced this pull request Nov 6, 2018
temp_unit.set_location necessary else pre and post teleport animation is played at the same place(arrival).

Closes #3684
@jostephd
Copy link
Member

jostephd commented Nov 6, 2018

@GregoryLundberg Thanks for merging. Could you fwdport to master? Or I will later.

@jostephd jostephd reopened this Nov 6, 2018
@jostephd
Copy link
Member

jostephd commented Nov 6, 2018

Sorry, I see it has been committed to both branches. My bad.

@jostephd jostephd closed this Nov 6, 2018
@newfrenchy83 newfrenchy83 deleted the patch-5 branch November 7, 2018 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Confirmed Issues that have been successfully reproduced by at least one developer. Regression Issues that were not present in previous releases. UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants