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

[Bug] replace GamePlayKit with aStar for path finding #17

Merged

Conversation

ampm
Copy link

@ampm ampm commented Jun 7, 2019

Hi @garvankeeley @jhugman , can you help to review this PR and to see if it is possible to merge it back into master branch?

Checkpoints

  • have tested it and now the pathfinding works stable.
  • Test with FireFox-iOS: SmokeTest works as well as with GamePlayKit(except of testAccountManagementPage, this case failed because of the code itself).

Test Result

image

Description

This PR has resolved #16

Rewind

One more thing I want to share with you is, in this branch(for reproducing the bug, if you remember)

https://github.com/ampm/MappaMundi/blob/ampm/reproduce_finpath_bug/

We repeatedly run testReproduceFindpathError ten times with the following code, and our app will relaunch before each invokes, we can reproduce the error easily, the right or wrong result comes alternatively.

override func invokeTest() {
       for time in 0...10 {
           print("this test is invoking: \(time) times")
           super.invokeTest()
       }
   }
   func testReproduceFindpathError() {
       navigator.nowAt(Screens.itemDetail)
       navigator.goto(Screens.foo5)
   }

But if we repeat it in this way(we run the test manually),

  • if the path is incorrect at the first loop, it will show us with 10 mistakes.
  • in a similar way, it will show us with 10 right results consistently if the path is right from begging.
func testReproduceFindpathError() {
       for _ in 0...10 {
           navigator.nowAt(Screens.itemDetail)
           navigator.goto(Screens.foo5)
       }
   }

@ampm ampm changed the title Ampm/replace GamePlayKit with a star for path finding [Bug] replace GamePlayKit with a star for path finding Jun 7, 2019
@ampm ampm changed the title [Bug] replace GamePlayKit with a star for path finding [Bug] replace GamePlayKit with aStar for path finding Jun 7, 2019
@Dev1an
Copy link

Dev1an commented Jun 7, 2019

Hello @ampm
Nice to hear that you are using my A* implementation. If you are interested, I made an SPM package that you can now easily depend upon using Xcode 11.

I also updated the code to use the new hashing function.
SPM dependency

@garvankeeley garvankeeley self-requested a review June 7, 2019 17:06
@garvankeeley
Copy link
Contributor

Hopefully we can upgrade to Xcode 11 in the coming months, but we want this project to be compatible with the build macs for Firefox iOS which is currently limited to Xcode 10.

@garvankeeley
Copy link
Contributor

Can you change the inclusion of A* to use:
Cartfile:

github "Dev1an/A-Star" "master"

Assuming 'master' is the working version here.
and add the Cartfile.resolved

Then include Carthage/Checkouts/A-Star/[the needed files] swift files in the mappamundi project as they are currently.

// MappaMundi
//
// Created by Yang, Roy on 2019/6/7.
// Copyright © 2019 Mozilla Corporation. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the mozilla licence header, thanks

Copy link
Author

Choose a reason for hiding this comment

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

sure, updated.

@Dev1an
Copy link

Dev1an commented Jun 7, 2019

I added a version tag so you can safely use all versions compatible with version 1.0.0
Cartfile:

github "Dev1an/A-Star" ~> 2.0.0

@ampm
Copy link
Author

ampm commented Jun 8, 2019

Can you change the inclusion of A* to use:
Cartfile:

github "Dev1an/A-Star" "master"

Assuming 'master' is the working version here.
and add the Cartfile.resolved

Then include Carthage/Checkouts/A-Star/[the needed files] swift files in the mappamundi project as they are currently.

Hi @garvankeeley , have integrated with Carthage.

@ampm ampm closed this Jun 8, 2019
@ampm ampm reopened this Jun 8, 2019
@ampm
Copy link
Author

ampm commented Jun 8, 2019

Hi @garvankeeley, have updated code, pls help to review it again, thanks.
Really appreciate @Dev1an's help.

@ampm
Copy link
Author

ampm commented Jun 8, 2019

hi @garvankeeley, pls on hold, I need to do more test to make sure it works well with FireFox-iOS.

@ampm ampm force-pushed the ampm/replace_GamePlayKit_with_aStar branch from 2382853 to d38434e Compare June 8, 2019 17:19
@ampm ampm force-pushed the ampm/replace_GamePlayKit_with_aStar branch from d38434e to 6c799f2 Compare June 8, 2019 17:22
@garvankeeley
Copy link
Contributor

I'll run all the Firefox iOS tests with this today and see how it goes.

@ampm
Copy link
Author

ampm commented Jun 10, 2019

I'll run all the Firefox iOS tests with this today and see how it goes.

sure, but pls let me know before merging, because of the Astar source code in MappamMundi is a little different with the version specified in Carhtage/Carthage.resolve files. (I copied them instead of using the checked out source at this moment.)

I have created a PR for it and waiting for @Dev1an to review it. Dev1an/A-Star#7 . once it is merged, I can update the Carthage file in this PR and link checked files into this project.

@garvankeeley
Copy link
Contributor

I'll wait until your PR is merged. For now, the test results are looking good on Firefox iOS.

@garvankeeley
Copy link
Contributor

Mind if I ping for a status update on this PR?

@ampm ampm force-pushed the ampm/replace_GamePlayKit_with_aStar branch from 214a85f to be26c47 Compare June 30, 2019 12:40
@ampm
Copy link
Author

ampm commented Jun 30, 2019

hi @garvankeeley , it's ready to merge now.

To apply this update in FireFox-iOS, I can create another PR with following actions for it.

1. add the dependency of
github "Dev1an/A-Star" ~> 3.0.0-beta-1
2. add the build output AStar.framework into the link binaries build phase of

  • L10nSnapshotTests
  • XCUITests

Thanks a lot.

@ampm ampm force-pushed the ampm/replace_GamePlayKit_with_aStar branch from 0ba8d43 to 617c392 Compare July 1, 2019 03:16
@garvankeeley
Copy link
Contributor

I'll land this and then update Firefox iOS.

@garvankeeley garvankeeley merged commit 1d17845 into mozilla-mobile:master Jul 3, 2019
winsmith added a commit to winsmith/browser-ios that referenced this pull request Jul 4, 2019
The Cartfile we inherited from Firefox contains this line:

`github "mozilla-mobile/MappaMundi"          "master"`

recently, MappaMundi was [updated to use the A-Star project](mozilla-mobile/MappaMundi#17) for pathfinding. This project, however, needs Swift 5. Hence, the build is broken.

To fix, pin the MappaMundi version to a commit before they introduce the A-Star algorithm.
winsmith added a commit to ghostery/browser-ios that referenced this pull request Jul 4, 2019
The Cartfile we inherited from Firefox contains this line:

`github "mozilla-mobile/MappaMundi"          "master"`

recently, MappaMundi was [updated to use the A-Star project](mozilla-mobile/MappaMundi#17) for pathfinding. This project, however, needs Swift 5. Hence, the build is broken.

To fix, pin the MappaMundi version to a commit before they introduce the A-Star algorithm.
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.

pathfinding error occasionally
3 participants