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

Refactor MakeWay #1687

Merged
merged 14 commits into from Apr 7, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented Mar 11, 2019

Depends on #1628

This currently passes all test cases in #1675 Except:

  • 1.2 - Collision parts pass, but the order of messages goes wrong later on. This is a deeper non-makeway related issue

Fix: #1675
Fix: #1612
Fix: #1606
Fixes a game hang reported in: #985

Retested: #884
Retested: #1082

@fdelapena fdelapena added this to the 0.6.1 milestone Mar 11, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch 2 times, most recently from 17285c2 to cf2ef84 Mar 11, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

This is now ready for review.

I believe I have covered every possible collision case for player and vehicles here.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Found a few more vehicle edge cases which have been fixed.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch from b6145c4 to 4cfb7e3 Mar 12, 2019

@fmatthew5876 fmatthew5876 referenced this pull request Mar 12, 2019
@carstene1ns

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@fmatthew5876 fmatthew5876 referenced this pull request Mar 13, 2019
3 of 3 tasks complete

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch 3 times, most recently from c28518b to 01722c5 Mar 13, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Even the better diff is very hard to follow, too much code moved around everywhere.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch from 01722c5 to 032add3 Mar 26, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

For this one I basically ripped out every collision related function in game_map.cpp.

Therefore for reviewing, I would suggest maybe just reading the final version of game_map.cpp and only using diffs for the other files touched.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I added a commit that not only fixed #937, but also test case 1.2 from #1675 mentioned in the original post!

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch 2 times, most recently from 465ced7 to 73b05fa Mar 29, 2019

fmatthew5876 added some commits Mar 7, 2019

Refactor MakeWay
* Refactor makeway logic to follow Cherry's description
  and trigger event updates at the right time
* Correct ordering of actions for diagonal movement
* Jump uses same MakeWay function to simplify logic and
  fix #1612
* Event layer=below tile graphic behavior
    * Highest event ID wins
    * If winning event tile is not above
         * Events and player - overrides chipset
         * Boat or Ship - always blocks
* Refactor IsPassableTile()
    * Better interface taking x,y
    * Support vehicles in one piece of code.
    * Handle tile event logic
* Vehicle cases
    * If player is boarded, calls makeway as vehicle (uses vehicle's
      through flag)
    * If player is boarded, other events collide against vehicle, not
      player
Refactor airship landling logic
* Remove IsPassableVehicle

fmatthew5876 added some commits Mar 8, 2019

Fix boat/ship embark collision checks
Adds Game_Map::CanEmbarkShip to perform tile collision checks
when attempting to embark.
Fix airship landing edge case
Create a lower tile that is:
* Passable by airship
* Landable by airship
* Not passable by player

And put an upper tile on it which is passable by player

Result: airship still cannot land there

The fix is to check lower tiles first in CanLandAirship()
Run parallel events everytime update is called
When a make way collision happens, RPG_RT will run
background interpreters every time, regardless of the
state of the processed flag.

Fix: #937
Fix looping maps
MakeWay function disallowed crossing looping map borders.
Take directions first, then round the coordinates.
Jump fixes
* When a jump fails, we don't change begin_jump_x or y
* Allow jumping across a looping map twice or more times

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_makeway branch from 73b05fa to bac79e3 Apr 3, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

When you ignore the diff and open old and new file instead and do full text search you notice that it's actually not that bad, mostly is really just code moved around (Sometimes with minor additions because of new behaviour matthew figured out), e.g.:

  • Map::AirshipLandOk & Vehicle::CanLand & Map::IsLandable -> Map::CanLandAirship
  • Vehicle::Update (partly moved to) -> Game_Vehicle::AnimateAscentDescent
  • Map::IsPassableVehicle is now integrated in Map::IsPassableTile
  • New func Map::IsPassableLowerTile uses code ripped out of Map::IsPassableTile

The Jump code was mostly rewritten, but this is a local thing so no big deal to review this.

So the ones with real changes are WouldCollide, MakeWayCollideEvent and MakeWay, looking deeper into them.

Edit: WouldCollide & MakeWayCollideEvent seems to be code taken from TestCollisionDuringMove

src/game_map.cpp Show resolved Hide resolved
src/game_event.cpp Show resolved Hide resolved
@Ghabry
Copy link
Member

left a comment

src/game_map.cpp Outdated Show resolved Hide resolved
src/game_map.cpp Outdated Show resolved Hide resolved
@Ghabry

Ghabry approved these changes Apr 4, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Maybe three things that could be retested are:

  • #1002 (Diagonal MakeWay)
  • #1079 (Unboard during Teleport)
  • #1349 (IsInVehicle-Timing issue with Airship)
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I retested all those games. All passed.

@carstene1ns carstene1ns merged commit cbb20f7 into EasyRPG:master Apr 7, 2019

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Wii (SDL1) Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:event_makeway branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.