-
Notifications
You must be signed in to change notification settings - Fork 198
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
swap s2geometry-node (C++) for s2-geometry (pure javascript) #165
Conversation
Legend |
Really Cool 👍 |
That library doesn't produce the correct cell for me. which is in the middle of the pacific ocean according to s2map.com. It should be around this location. |
I'm using this PR and works for me. Thanks! On Fri, 29 Jul 2016, 05:18 Toeler, notifications@github.com wrote:
|
It's either a padding bug or an issue with the GPS conversion (an overflow or wrap perhaps). I've opened an issue here https://github.com/coolaj86/s2-geometry-javascript/issues/6 and I'll probably be able to find and fix it very shortly. Thanks for the report! :D |
The good news is that there was a very simple padding issue, fixed here: The bad news is that there's also an issue with the Lat/Lng conversion, which is a little deeper, down in code that I didn't write. I'll see what I can do. :) |
@Toeler Can you use another library to figure out what the IJ, UV, and ST values should be at that location? I get I was able to see that the Face and XYZ conversion is consistent with s2geometry-node, so those aren't the problem. Update: I modified https://github.com/golang/geo to print out the debug values:
|
I don't have any other S2 libraries that work, just your javascript one. |
Using npm install I still see s2geometry-node, is it because of the issue with s2-geometry? EDIT: Nevermind, using https://github.com/Daplie/Pokemon-GO-node-api.git#master as repository solved the issue. |
@raz3r There is an issue though https://github.com/coolaj86/s2-geometry-javascript/issues/8 and I would like some help with it. It's something any code monkey can solve, it just requires the endurance of |
@coolaj86 hey there. Im curious, if the "implementation that works" that you linked to before ( By using something like this (theres probably other ways of doing it too): |
@cokeeffekt @Toeler @Armax @billyriantono @breandr I fixed the bug in my library. This is ready to merge as of s2-geometry v1.2.6. I've exhaustively tested every location on earth at level 15 and all generated values exactly match with s2geometry-node: |
This was the fix: https://github.com/coolaj86/s2-geometry-javascript/commit/68274eb677be0338984b0c7d59290ec323c47e67 Aside from the original padding error, it can essentially be boiled down to a very simple change:
No big deal though... only half of the earth was getting wrong positions when it was hard-coded to 'a'... hahaha |
@coolaj86 i just tried your s2-geometry fork and it seems to work! Is it ready to use?? Good job!! :D |
@segura2010 Yes, this is ready to merge. I've tested over 2 million random locations which all have passed. The only bug reports I've gotten since the update were people using old versions. |
@syrys the point is to get away from C++ complied code and use pure JavaScript to reduce complexity of the install. Otherwise using |
@coolaj86 just for clarification - this will completely remove the need for the C++ dependency for pure javascript (tested and working)? Would like to know before I swap out for my repo |
@d-pollard it does, we already use it here https://github.com/stoffern/pokemongo-api and it works completely fine. |
I totally vote for it! |
@Stoffern check the new README and see if that usage of |
We have our own calculation for getting the neighbour cells |
What's the holdup on getting this merged in? All necessary functionality has been implemented. |
your commit is conflict need to resolve first 😄 |
Would be easier to just send new PR |
Big +1 on getting this merged. I'm working to get this 100% client-side compatible. It requires users to install a small browser extension to bypass CORS and set Origin header to 'niantic', but seeing as we've already seen Niantic block the major cloud providers, it's likely they'll soon continue with blocking the smaller ones. I have it working to the point of the second request in the PTC auth flow, but this is returning a 200 with no |
Oh, it looks like the branches got crossed. It's showing commits from my other PR. Here it is: #222 |
I also just did a force push to this original PR which also fixed the merge conflict. |
@breandr do you have a repo for the browser-only fork? Or is it commercial? |
Can't remember if this includes all the latest? I was just hacking, waiting for this PR to get merged as it's obviously a big part of it. https://github.com/breandr/pokemon-go-extension Once this is merged, I will clean up and send PR to this repo (need to get auth working as well). I'll need to find an alternative to |
All necessary changes for implementing s2-geometry in pure javascript.
Tested in known locations and compared to s2geometry-node results
Addresses issues #147 #96 #70 #63 #64 #60
@cokeeffekt @d-pollard @breandr @hunterjm @dotomaz @billyriantono @Armax
Try it out now
Pre-packed instructions that even your sister can figure out (probably):
Simple copy-paste instructions for techies, like you: