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

geodesicConvertLine: calculate seg count based on distance #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Artoria2e5
Copy link

@Artoria2e5 Artoria2e5 commented May 28, 2024

Suggested by @johnd0e in IITC-CE/ingress-intel-total-conversion#232 (comment). Should work for now, at least until we switch to the shinier Geodesic addon by henrythasler.

(About a potential switch: henrythasler's thing uses a WGS84 ellipsoid. IITC-CE/ingress-intel-total-conversion#232 (comment) says stock uses https://github.com/chrisveness/geodesy, but it's unclear whether stock uses the spherical formula from Chris or the WGS84 ellipsoid one. If it's the former [quite likely!], henrythasler's thing won't work for us.)

Suggested by @johnd0e in IITC-CE/ingress-intel-total-conversion#232 (comment). Should work for now, at least until we switch to the shinier Geodesic addon by henrythasler.
@johnd0e
Copy link
Collaborator

johnd0e commented May 28, 2024

I am not sure that lib by henrythasler is really better. In fact, I think rather opposit: our lib is tiny and easily tunable.

@Artoria2e5
Copy link
Author

Artoria2e5 commented May 28, 2024

Yeah, I just did some re-thinking and came to the conclusion that it might be doing WGS84 unnecessarily -- which might even take us further away from Stock.

Uh JD, what's your opinion on this distance thing? I think the idea that the dLng matters more is still valid -- maybe some sort of weighing factor would work? As in

    var distance = Math.acos(
      Math.sin(lat1) * Math.sin(lat2) +
      Math.cos(lat1) * Math.cos(lat2) * Math.cos(-dLng));
    var LNG_WEIGHT = 0.9;
    var segments = Math.ceil((distance * (1-LNG_WEIGHT) + LNG_WEIGHT * Math.abs(dLng)) * earthR / segmentLength);

@johnd0e
Copy link
Collaborator

johnd0e commented May 28, 2024

I think the idea that the dLng matters more is still valid -- maybe some sort of weighing factor would work? As in

I believe that you are right. But I'm not sure about exact formula should we implement, as this weighting factor seem to not be linear.

@Artoria2e5
Copy link
Author

Artoria2e5 commented May 29, 2024

Hmm. I think we can still use linear approximation to establish some bounds for how little we can weigh the overall distance. Recall in the 232 comment that you stated, with a full distance, "this fixes case with set of given 3 portals, even if we use far greater coefficient: var segmentLength = 1000000". Since our current setting is 5000, it follows that the following would still work for the three portals:

DIST_WEIGHT = 5000 / 1000000
var segments = Math.ceil((distance * DIST_WEIGHT + Math.abs(dLng) * (1-DIST_WEIGHT)) * earthR / 5000);

Because Math.abs(dLng) * (1-DIST_WEIGHT) has to be non-negative, this new segment count is definitely no smaller than than your known-good Math.ceil(distance * earthR / 1000000);.

Maybe 5000 / 1000000 = 0.005 is a little too aggressive, but something along these lines will work. Time to find some long north-south links on the map...

@johnd0e
Copy link
Collaborator

johnd0e commented May 29, 2024

That is why I have refactored the code - to make some parts easily overridable.
You just need to put your modifications into custom micro-plugin, and than make some experiments.

And it is necessary to profile the code, as performance is also the criteria.

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.

2 participants