Flight Fixes #963

Simplicitee · 2018-07-10T01:32:41Z

Fixes

Fixes Flight causing fly glitch w/other abilities
Fixes Flight persisting without Air element

Misc. Changes

Adds Override annotation to event methods
Removes accessibility to more of FlightHandler and sub-classes

WaterArms and FireWheel fixes (#950) Merge

Mergie

Merge

merge

jayoevans · 2018-07-10T07:42:56Z

src/com/projectkorra/projectkorra/airbending/flight/FlightMultiAbility.java


 public class FlightMultiAbility extends FlightAbility implements MultiAbility{

 	public static Map<UUID, UUID> requestedMap = new HashMap<>();
 	public static Map<UUID, Long> requestTime = new HashMap<>();

 	private static Set<UUID> flying = new HashSet<>();
-	private boolean canFly, hadFly, hadGlide;
+	private boolean hadGlide;
+	private Flight flight;


You should never store an instance of Flight yourself. FlightHandler will manage all of this.

In the FlightMultiAbility I set the states of flying various times depending on the mode. In the ending mode, I want them to have whatever the state of flying was before using the ability or starting to fly. I store it so I can access those stored states whenever they go to ending mode (since it's a mutliability, they can switch between binds whenever, need to check every iteration of progress). I do this because FlightHandler doesn't manage it in the way I said, not when I need it to. So yes I do need to store the instance of Flight, or else why does the createInstance method return Flight at all, if I'm not intended to store it?

In the FlightMultiAbility I set the states of flying various times depending on the mode.

That's fine, though you shouldn't be doing that from the Flight instance. FlightHandler works by storing a player's flight state for multiple identifiers and does not reverting until all instances have been removed. If you were want to give them flight, use the createInstance method and then do whatever you want with the player (setFlying(true/false), setAllowFlight(true/false)). Then when you're done, call the removeInstance method. If they have no other instances of Flight outstanding, and only if that is true, it will revert their flight state back to what it was when you (or their first flight instance) called createFlight. If you are manually setting their flight back when their FlightMultiAbility is removed, that will also prematurely revert their flight for another ability.
Eg. A player is hit by AirSwipe, Tornado, AirBlast, etc. just before their FlightMultiAbility ends. Keep in mind that the whole point of FlightHandler, and the reason we had so many fly glitches before, was because abilities did not communicate player's flight states correctly. FlightHandler is designed to save a player's fly state, and then when ALL abilities are done using flight, revert it back to what it was. You're completely defeating the purpose of using FlightHandler by manually reverting it.

or else why does the createInstance method return Flight at all, if I'm not intended to store it?

It doesn't actually need to return the Flight instance, and I'll change that in a future PR.

If you are manually setting their flight back when their FlightMultiAbility is removed, that will also prematurely revert their flight for another ability.

This doesn't make sense. It shouldn't revert flight for other abilities if I just change their flying states, even if it is back to what it was before the flight. Nowhere would the code cause other abilities flight to prematurely revert.

It doesn't actually need to return the Flight instance, and I'll change that in a future PR.

I think this should be left how it is, because it is useful to access flight (since you can't actually change anything about the flight itself)

It shouldn't revert flight for other abilities if I just change their flying states, even if it is back to what it was before the flight. Nowhere would the code cause other abilities flight to prematurely revert.

If another ability has someone set flying and you turn it off, or vice verse, you are prematurely reverting that abilities flight.

And it should be? FlightMultiAbility should take priority

No it shouldn't. If someone is hit with AirBlast a moment before ending their FlightMultiAbility, doing what you're doing will also remove AirBlast's Flight instance, likely causing the player to get kicked for flying.

jayoevans · 2018-07-10T07:43:58Z

src/com/projectkorra/projectkorra/airbending/flight/FlightMultiAbility.java

 		MultiAbilityManager.bindMultiAbility(player, "Flight");
-		hadFly = player.isFlying();
-		canFly = player.getAllowFlight();
+		flight = ProjectKorra.flightHandler.createInstance(player, "FlightMultiAbility");


Use getName() or store a final String with the value of "FlightMultiAbility" instead

Doesn't make much difference what identifier I use tbh, but sure I'll change it

Yeah, I just think it's more aesthetically pleasing, and since it's being used in multiple locations it makes sense to do so imo

jayoevans · 2018-07-10T07:44:36Z

src/com/projectkorra/projectkorra/airbending/flight/FlightMultiAbility.java

@@ -215,8 +243,8 @@ public void run() {
 			player.setFlying(true);
 		} else if (mode == FlightMode.ENDING) {
 			player.setGliding(hadGlide);
-			player.setAllowFlight(canFly);
-			player.setFlying(hadFly);
+			player.setAllowFlight(flight.getCouldFly());


Again, FlightHandler will manage the reversion of a player's flight state. These lines need to be removed.

Refer to first comment

jayoevans · 2018-07-10T07:45:08Z

src/com/projectkorra/projectkorra/airbending/flight/FlightMultiAbility.java

@@ -289,8 +317,7 @@ public void remove() {
 		if (player.isOnline() && !player.isDead()) {
 			player.eject();
 		}
-		player.setAllowFlight(canFly);
-		player.setFlying(hadFly);
+		ProjectKorra.flightHandler.removeInstance(player, "FlightMultiAbility");


This line will manage the player's flight state, but again change "FlightMultiAbility" to getName() or a final String with the same value.

Refer to second comment

jayoevans · 2018-07-10T07:45:32Z

src/com/projectkorra/projectkorra/util/FlightHandler.java

@@ -199,6 +199,14 @@ public Player getPlayer() {
 		public Player getSource() {
 			return source;
 		}
+
+		public boolean getCouldFly() {


All of these changes can go given my previous comments.

These should stay regardless. It is now and could be useful in the future.

Given the top comment, there's no need a third party would need to access this information. Like how I have createInstance returning a Flight instance where it shouldn't be stored, these methods are asking for people to access them when they don't need to.

But there is no harm in them accessing them, since they can't change them.

Requested changes have been completed

ChristopherWMM · 2018-07-14T21:55:30Z

src/com/projectkorra/projectkorra/airbending/flight/FlightMultiAbility.java

@@ -128,6 +148,11 @@ public void progress() {
 			return;
 		}

+		if (!bPlayer.hasElement(Element.AIR)) {


This check is not necessary as of #964.

Not necessary after #964.

Simplicitee added 14 commits June 8, 2018 20:39

WaterArms and FireWheel

66817e0

Fix WaterArms more

065696a

Merge pull request #2 from ProjectKorra/wip

c4ea34e

WaterArms and FireWheel fixes (#950) Merge

Merge pull request #3 from ProjectKorra/wip

63f3fa1

Mergie

Earthbending balancing and improvements

3611f31

Fix damageThreshold for EarthGrab

a7c8254

Merge pull request #4 from ProjectKorra/wip

8d00a56

Merge

Update EarthGrab description

6907229

Merge pull request #5 from ProjectKorra/wip

01d98b7

Merge

Flight bugs, HealingWaters changes

4a20a5a

Undo HealingWaters changes

d0d28b0

conflicts

301e9a0

conflicts pt 2

6a8cb41

Merge pull request #6 from ProjectKorra/wip

443f8f5

merge

jayoevans previously requested changes Jul 10, 2018

View reviewed changes

FlightHandler changes

e607deb

ChristopherWMM reviewed Jul 14, 2018

View reviewed changes

Remove redundant check in FlightMultiAbility

738b475

Not necessary after #964.

ChristopherWMM merged commit 1debe50 into ProjectKorra:wip Jul 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flight Fixes #963

Flight Fixes #963

Simplicitee commented Jul 10, 2018 •

edited

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018 •

edited

Simplicitee Jul 10, 2018 •

edited

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

jayoevans Jul 10, 2018

Simplicitee Jul 10, 2018

ChristopherWMM Jul 14, 2018

Flight Fixes #963

Flight Fixes #963

Conversation

Simplicitee commented Jul 10, 2018 • edited

Fixes

Misc. Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayoevans Jul 10, 2018 • edited

Choose a reason for hiding this comment

Simplicitee Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simplicitee commented Jul 10, 2018 •

edited

jayoevans Jul 10, 2018 •

edited

Simplicitee Jul 10, 2018 •

edited