Changes for IITC #1

modos189 · 2019-04-28T20:17:28Z

On one hand, this change is specific to IITC, and on other hand - for greater similarity with the leaflet. Perhaps we should create a parameter to determine whether the container should be destroyed.
See: b6c228a

* leaflet.canvas-markers.js: - include source IITC-CE/Leaflet.Canvas-Markers@e0bc449 IITC-CE/Leaflet.Canvas-Markers#1 (IITC-CE/Leaflet.Canvas-Markers@e0bc449) * rbush.min.js - required by leaflet.canvas-markers.js https://unpkg.com/[email protected]/rbush.min.js

johnd0e · 2019-04-29T17:27:52Z

a52cbaf

Nice job!

modos189 · 2019-04-29T22:13:58Z

Maybe you'll be able to finish options.padding support. Ornaments are constantly shifting: either by moving the map or changing the screen size

johnd0e · 2019-04-30T06:19:11Z

Ornaments are constantly shifting

We need padding to solve this, or this issue caused by unfinished padding support?

modos189 · 2019-04-30T06:29:19Z

It's not ready. Maybe I should have sent it to a separate branch.
Padding is needed to draw portals that are outside the visible part of the screen, but close to it, just like in Leaflet and IITC. I have implemented this, but there are some conflicts when moving the map. Ornaments move more than portals.

johnd0e · 2019-04-30T07:02:08Z

As this feature is not blocker it makes sense to put it separate branch.

And tell me more about 419b1ab: what is 'atypical use'?

modos189 · 2019-04-30T10:34:08Z

I was able to fix it. Padding works correctly.

what is 'atypical use'?

Our code tries to remove those markers which may not exist.

https://github.com/IITC-CE/ingress-intel-total-conversion/blob/256f9c53a866a254b40e40b1f766e58b4d2a89f4/code/ornaments.js#L69-L73

That's why I added additional checks to correct the errors.

src/plugin/leaflet.canvas-markers.js

johnd0e · 2019-04-30T12:26:39Z

I was able to fix it. Padding works correctly.

Great!
But check upper comment please ^^^:

johnd0e · 2019-04-30T12:57:48Z

src/plugin/leaflet.canvas-markers.js

+            // e.g. 0.1 would be 10% of map view in each direction
+            padding: 0.1
+        },
+


Shouldn't we inherit this value from map renderer?

In Leaflet padding value is specified in Renderer class (var Renderer = Layer.extend({) and is not overridden.
It would be useful to assign an indent equal to that in window.map in iitc, but this is hardly possible.

It would be useful to assign an indent equal to that in window.map in iitc, but this is hardly possible.

Why hardly? I suppose we just need map._renderer.options.padding.
Or map.getRenderer({options:{}}).options.padding.

Then in our boot.js we can do like this:

L.CanvasIconLayer.mergeOptions({ padding: map._renderer.options.padding` });

johnd0e · 2019-04-30T14:37:10Z

src/plugin/leaflet.canvas-markers.js

@@ -24,7 +24,8 @@ function layerFactory(L) {
            this._draw();
        },
        onRemove: function () {
-            this._destroyContainer();
+            // TODO
+            // this._destroyContainer();


Call this._destroyContainer() is unique to this plugin and redundant

Currently L.CanvasMarkerLayer has dual nature:

L.Layer, from which it's inherited initially.

And after Spaction@153525e it has some features of L.Renderer (but still is not inherited from it)

I can guess that mentioned commit was just a step to make true renderer which can be used transparently (#2).
But in it's current (perhaps unfinished) state L.CanvasMarkerLayer is rather layer, so it should'n destroy it's container on remove.

I wonder if we've chosen right fork to continue..

johnd0e · 2019-05-01T09:52:08Z

Our code tries to remove those markers which may not exist.

removeMarker was broken anyway.

Also I am going to fix our code, so we do not need additional checks in upstream.

Although, those checks may make sense in general (but not in this branch).

This reverts commit 419b1ab. #1 (comment)

src/plugin/leaflet.canvas-markers.js

johnd0e · 2019-05-01T15:23:11Z

@modos189
I am going to clean up and rewrite this branch, tell me when you ready.
Separate branches for all features already created (todo: a52cbaf)

johnd0e · 2019-05-02T14:58:55Z

Please check comments regarding padding option.
I have implemented fixes in padding-option branch, but not tested yet.

src/plugin/leaflet.canvas-markers.js

johnd0e · 2019-05-02T17:06:37Z

My fault, thank you for pointing out.

modos189 · 2019-05-02T18:19:51Z

I saw you similar change in padding-option branch. But there is an error: instead of layerPointToContainerPoint you should use containerPointToLayerPoint

johnd0e · 2019-05-03T06:37:19Z

I saw you similar change in padding-option branch

I've just updated that branch with your last commit.
Let's continue padding-related development there.

johnd0e · 2019-05-03T08:20:27Z

As all needed for IITC features are now separated between own branches, all we need - merge them together, like this:

git checkout master
git checkout -b changes-for-IITC
git merge --no-ff fixes canvas-resize layer-features-back padding-option
git push --force

I've already done this in test branch, and about to rewrite changes-for-IITC branch with the content of test.

All future updates should go to feature branches, here we'll need merge them together only.

P.S.
And I suppose we need to open corresponding PR's in Spaction's repo.

johnd0e · 2019-05-06T11:41:42Z

@modos189
I finished with all branches except padding-option.

I am unable to check it, as I just do not understand how it should function.
Let's start from related issue: IITC-CE/ingress-intel-total-conversion#184

`this._markers` is undefined in `_searchPoints`, called from event listeners. Now we check markers existence in the beginning of handlers. Note 1: Could be fixed in some another place: e.g. check `_markers` in `_searchPoints` ...or initialize (empty) `_markers` on CanvasMarkerLayer init Note 2: `!this._map` removed from condition in `_onMouseMove:` 'cause it's never should be true as we detach listeners on remove from map

… map

Original intentions of that refactoring is not clear, perhaps it was intermediate step before transforming into true Renderer. But in current state it is not actually Renderer, so we need to revert some parts, to continue using it as independent Layer. Notes: `CanvasMarkerLayer` is still independent entity which does not fit flawlessly into Leaflet ecosystem. It would be much more convenient if instead of `CanvasMarkerLayer` we have `CanvasMarker`, able to be used as direct and transparent replacement for `L.Marker`. Though currently I am not sure how to do it best: - may be we need dedicated Renderer class - or may be it can be simpler, and all we need - `CanvasMarker` inherited from `L.Path`.

Was protected in marker add time, but not on zoom/pan Sample: ``` VM2992:16252 Uncaught DOMException: Failed to execute 'drawImage' on 'CanvasRenderingContext2D': The HTMLImageElement provided is in the 'broken' state. at NewClass._drawImage (<anonymous>:16252:23) at NewClass._drawMarker (<anonymous>:16239:22) at <anonymous>:16200:22 at Array.forEach (<anonymous>) at NewClass._draw (<anonymous>:16179:16) at NewClass._redraw (<anonymous>:16077:18) at NewClass.fire (<anonymous>:1867:11) at NewClass._move (<anonymous>:5506:9) at NewClass._onZoomTransitionEnd (<anonymous>:5963:8) ```

This was originally meant but missed (see _full.js / _standalone.js)

…adding-option' into test

modos189 · 2019-05-22T17:54:19Z

It's all working now.

johnd0e · 2019-05-22T18:13:54Z

@modos189
I am not sure that 2 latest commits are needed.
See test branch (as I said in previous message)

modos189 · 2019-05-22T18:18:05Z

In IITC-CE/ingress-intel-total-conversion#181 which branch is being used? I used that version and I thought it was from test branch

up: Indeed, these changes already exist in the test branch

johnd0e · 2019-05-22T18:38:02Z

Do you still need this branch? If not then I will rewrite it with actual commits.

modos189 · 2019-05-22T18:41:15Z

No, you can rewrite

johnd0e · 2019-05-24T14:59:34Z

Please check this comment: 71eab75#commitcomment-33502466

modos189 · 2019-06-27T21:25:54Z

I'm totally confused in branches, so I created a new branch for onresize method: https://github.com/IITC-CE/Leaflet.Canvas-Markers/tree/onresize

Merge after this: #1 (comment)

johnd0e · 2019-06-29T13:47:40Z

I'm totally confused in branches

We have independent branch for every new feature here. We keep it so it will be easy to make PR's to parent repo in the future.

And all that branches merge together here, in changes-for-IITC - it is not independent branch.

So to complete our work here we should finish last branch, for what we have concerns: padding-option.
I think it is not ok, check comments for f451114

modos189 · 2019-07-09T19:36:04Z

@johnd0e 1d88dcf

johnd0e · 2019-07-10T08:01:40Z

@johnd0e 1d88dcf

Good! But see question there.

Also question about _updateTransform left unanswered in f451114

Fix bugs

modos189 mentioned this pull request Apr 28, 2019

Fix performance drop when displaying a large number of ornaments IITC-CE/ingress-intel-total-conversion#181

Merged

johnd0e reviewed Apr 30, 2019

View reviewed changes