IITC-CE / ingress-intel-total-conversion

intel.ingress.com total conversion user script with some new features. Should allow easier extension of the intel map.
https://iitc.app
ISC License
284 stars 110 forks source link

Expanding functionallity of findPortalGuidByPositionE6 #418

Open MysticJay opened 3 years ago

MysticJay commented 3 years ago

effected plugin: CACHE-PORTALS-ON-MAP

The function findPortalGuidByPositionE6 is converting an E6-Position in to a guid. This is only possible for portals in the current viewport for selected or at zoom level "all portals", as portal details need be loaded. The plugin CACHE-PORTALS-ON-MAP caches the portal details, but does not allow the function findPortalGuidByPositionE6 to benefit from that information.

The code to add the functionality is ready and available from PR#389 (which was reverted)

The discussion is about how to inject the code into the existing findPortalGuidByPositionE6

The proposed code uses code injection by calling the original function. The alternative would be a hook, but that slows down the processing as all hooked functions would need to be executed without the possibility to stop execution if the guid is found. A third alternative would be to overwrite the core function. But this would cause changes to the core function never to get active, when CACHE-PORTALS-ON-MAP is loaded.

OFFLE, a 3rd-party plugin, is also providing caching functionality for portal data. OFFLE has also been changed to use code injection to extend the core function findPortalGuidByPositionE6.

MysticJay commented 3 years ago

UNIQUES040 #416 would greatly profit from extending findPortalGuidByPositionE6

MysticJay commented 3 years ago

earlier comments from @johnd0e https://github.com/IITC-CE/ingress-intel-total-conversion/pull/388#discussion_r544947362

MysticJay commented 3 years ago

@MysticJay There is one other option: cachePortals is a very small plugin. we could even add the code to the core and avoid all the discussion.

johnd0e commented 3 years ago

The plugin CACHE-PORTALS-ON-MAP caches the portal details

I have some small but rather important clarifications about that's plugin caching implementation:

So my conclusion: the plugin is not so useful as someone would imagine.

UNIQUES040 #416 would greatly profit from extending findPortalGuidByPositionE6

Not really, because that cache is tooo small.

cachePortals is a very small plugin. we could even add the code to the core and avoid all the discussion.

That's why answer is "No way". It is just pointless. But we can provide special api instead, to be able to benefit from any external plugin.

OFFLE, a 3rd-party plugin, is also providing caching functionality for portal data.

And also many others plugins. That's why I'd prefer some clean api solution for all them.

johnd0e commented 3 years ago

The alternative would be a hook, but that slows down the processing (1) as all hooked functions would need to be executed without the possibility to stop execution if the guid is found(2).

1) Baseless without real measurements 2) It depends on implementation. Yes, current hook loop is uninterruptable, but listener itself can check: if data is already available then do nothing and just exit.

johnd0e commented 3 years ago

UNIQUES040 #416 would greatly profit from extending findPortalGuidByPositionE6

And one more clarification: as currently implemented, portal guid MUST be in cache already, otherwise that data is just lost. Again my conclusion: considering that cache is extremely small (as for now), this code is much less effective than someone would imagine.

The more effective strategy would be save those unresolved latlngs for later checking.

McBen commented 3 years ago

is https://github.com/IITC-CE/ingress-intel-total-conversion/blob/master/core/code/data_cache.js not already doing the same thing?

with the exception that the data isn't rendered and not stored for 12hours - but with improved memory managment.

MysticJay commented 3 years ago

@johnd0e From the discussion I agree. Cache portals on map seems less usefull in resolving guid.

uniques is already remembering unresolved portals missedLatLngs

OFFLE saves guids even on reload, so for the time we can leave ir with that.

johnd0e commented 3 years ago

is https://github.com/IITC-CE/ingress-intel-total-conversion/blob/master/core/code/data_cache.js not already doing the same thing?

Well, window.DataCache is merely general-purpose class, that can be used for anything. In fact, it is instantiated as window.MapDataRequest (not related to subject at all), and cache of window.portalDetail.

window.portalDetail is not very useful too, because it stores only portals for those we have full details (results of window.portalDetail.request).

But indeed, it looks like for cache-portals-on-map we can reuse window.portalDetail.get instead of reimplementing one more cache instance.

Edit: Perhaps it also possible to use window.DataCache for pushPortalGuidPositionCache/findPortalGuidByPositionE6 (requires some refactoring though).

MysticJay commented 3 years ago

@johnd0e The problem is not getting something out of the cache if guid is know, but to find the guid when you only have LatLonE6 from the COMM. Also we do not need the full details.

johnd0e commented 3 years ago

@MysticJay Obviously. But my message was not about it.

johnd0e commented 3 years ago

Should this issue stay opened?

MysticJay commented 3 years ago

@johnd0e this is implemented in Offle afaik and cache and uniques were also involved. leave open.