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
285 stars 110 forks source link

padding option #184

Closed johnd0e closed 4 years ago

johnd0e commented 5 years ago

We have old code changing L.Path.CLIP_PADDING, but this property is not used in current Leaflet, there is Renderer option padding instead.

Question: should we ever change defaults?

modos189 commented 5 years ago

Now padding can be specified using option renderer: L.canvas({ padding: 0.5 }),

It would be useful to specify a large padding if portals were reloaded while the map was being moved, rather than after releasing the mouse.

But now increase of padding is confusing.

johnd0e commented 5 years ago

Previously our padding varied, and actually (in case of canvas) was decreased too.

There was some comment about performance.

But now increase of padding is confusing.

Why?

modos189 commented 5 years ago

This code was added to this commit: https://github.com/iitc-project/ingress-intel-total-conversion/commit/69f3fb211b2c8b8d511743f1c151803bc34e6830

If I understood correctly, SVG is drawn by browser only inside the visible party of window. This allows to specify padding as 100%.

I tried to make a padding 100% and it's confusing. It's hard to know where the edge of downloaded portals is to wait for new portals to load.

This seems to be the subject of separate issue, but I want to load portals at the same time as moving map.

johnd0e commented 5 years ago

tried to make a padding 100% and it's confusing. It's hard to know where the edge of downloaded portals is to wait for new portals to load.

It's implied that when we have 100% overclip — we rarely need to wait portals to load, as it occurs in background. I don't know what is true, but from mentioned commit message I conclude that large overclip was intended to keep already loaded entities on map (do not remove them when they appear offscreen, in case if we may need to pan back).

And it's interesting why so low padding value for canvas mode (0.02)... May be at that time canvas was slower them SVG? Or it was too buggy?

johnd0e commented 5 years ago

This seems to be the subject of separate issue, but I want to load portals at the same time as moving map.

Well, loading delay is intentional, to lower ingress servers usage. I can be wrong (haven't yet studied that part of code), but it seems that current data refreshing algorithm is not optimized, and even for small movement can initiate full map refresh. And you are right, it is worth separate issue.

Edit: full refresh also can be intentional, to keep visible map in consistent state.

johnd0e commented 5 years ago

Returning to our subject: I will prepare PR for padding. As for optimal value I suppose we need more experiment to find it empirically.

modos189 commented 5 years ago

Now padding 0.1 (10%) by default, why not

johnd0e commented 5 years ago

As for optimal value I suppose we need more experiment to find it empirically.

Well, I've tried: https://github.com/johnd0e/ingress-intel-total-conversion/commit/f87d2d6d8659eda85046fef9e0972c0c6bfda088

But I do not see any difference, even after setting window.PADDING to 1. What am I doing wrong?

modos189 commented 5 years ago

It will work, but

note: before i set window.PADDING as 1.

  1. L.Renderer.mergeOptions({
      padding: window.PADDING
    });

    it only changes padding for a regular map.

screenshot ![изображение](https://user-images.githubusercontent.com/1785196/57357806-81424c80-717c-11e9-9a65-f60a53d3dd2c.png)

need to add also

    L.Layer.mergeOptions({
      padding: window.PADDING
    });
  1. It would work now, but padding is rewritten by value 0.1. Need to remove padding: 0.1 from options in CanvasIconLayer.
screenshot ![изображение](https://user-images.githubusercontent.com/1785196/57358246-aaafa800-717d-11e9-8662-2fdbbd8a83b2.png)
  1. As you can see in screenshot, size of canvas has now increased, but ornaments are drawn only those that are in the visibility area. It`s a bug.

https://github.com/IITC-CE/Leaflet.Canvas-Markers/blob/e702fba59b164ac0fa1c9588e1d3804c8e089155/src/plugin/leaflet.canvas-markers.js#L207

need change from 0.1 to self.options.padding

now all ok

Details ![изображение](https://user-images.githubusercontent.com/1785196/57358633-87d1c380-717e-11e9-87fb-030b70f30209.png)

But Map does not extend the Renderer, I do not know how set padding option on Renderer changes padding on Map.

johnd0e commented 5 years ago

But Map does not extend the Renderer, I do not know how set padding option on Renderer changes padding on Map.

We just need to do it manually. I will, as soon as I understand how padding should work in general.

need change from 0.1 to self.options.padding

Done in feature branch (in other places I will update later).

need to add also

    L.Layer.mergeOptions({
      padding: window.PADDING
    });

Thanks, I will try now (thought It looks weird - why change it in two places separately).

johnd0e commented 5 years ago

But I do not see any difference, even after setting window.PADDING to 1. What am I doing wrong?

It seems I was doing right.

need to add also

    L.Layer.mergeOptions({
      padding: window.PADDING
    });

I do not think so. E.g. evaluate in console: portals[selectedPortal]._renderer.options.padding

johnd0e commented 5 years ago

But Map does not extend the Renderer, I do not know how set padding option on Renderer changes padding on Map.

We have here several alternatives:

Update I've chosen last option.

johnd0e commented 5 years ago

It's hard to know where the edge of downloaded portals is to wait for new portals to load.

In padding area iitc draws only that features, that are currently (partly) displaying in screen boundaries. That includes links, fields, and corresponding placeholder portals.

If it seems confusing... Well, we already have requests progress indicator in bottom right corner. May be we should make it bigger, like in stock intel.

modos189 commented 5 years ago

I'd prefer to load data when moving map. Just like it's done for tiles of map layer

johnd0e commented 5 years ago

I'd prefer to load data when moving map. Just like it's done for tiles of map layer

And now it is done exactly like that (other is proposed only in #204). So look what I mean:

  1. padding is not related to data loading
  2. entities like links and fields can extend beyond current view - that where large padding comes in help: we see such links/fields not truncated
  3. why should we differ from stock intel for the worse?