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

Properly update display internals when updating bounds #50

Merged
merged 2 commits into from
May 22, 2020

Conversation

evansiroky
Copy link
Contributor

@evansiroky evansiroky commented May 21, 2020

Some internals in the display class weren't properly getting updated when the map bounds were being changed. This was causing crazy-looking paths being drawn in certain cases.

Fixes opentripplanner/otp-react-redux#159

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #50 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #50      +/-   ##
=========================================
- Coverage    0.18%   0.18%   -0.01%     
=========================================
  Files          37      37              
  Lines        3181    3182       +1     
  Branches      660     660              
=========================================
  Hits            6       6              
- Misses       2612    2613       +1     
  Partials      563     563              
Impacted Files Coverage Δ
lib/transitive.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da5f26...7f87047. Read the comment docs.

@@ -178,6 +178,7 @@ export default class Transitive {
if (!this.display) return
const smWestSouth = sm.forward(llBounds[0])
const smEastNorth = sm.forward(llBounds[1])
this.display.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding a comment here related to this issue. Otherwise, incredible work finding the needle in the haystack!

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Incredible work. I still need to test this though. Can you post steps on your setup to test?

@landonreed landonreed assigned evansiroky and unassigned landonreed May 21, 2020
@evansiroky
Copy link
Contributor Author

Steps to reproduce/verify:

  1. Setup local OTP (use this Graph.obj.zip)
  2. Checkout commit b32efd4b961cc9f9ecf02fddc26b1ca4feb7abb3 of trimet-mod-otp
  3. See discussion on teams about config
  4. Start trimet-mod-otp
  5. Plan a trip from Terrace Dr, Carmel, NY, 10512, USA to 37 Huguenot Rd, Carmel, NY, 10512, USA departing after June 1 in the morning.
  6. Observe transitive weirdness
  7. Copy line of code into trimet-mod-otp/node_modules/transitive-js/build/transitive.js in the setDisplayBounds method.
  8. Should be fixed.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky May 21, 2020
@landonreed landonreed merged commit 06c5750 into master May 22, 2020
@landonreed
Copy link
Contributor

🎉 This PR is included in version 0.13.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transitive rendering bug
3 participants