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

LIcon encapsulation breaks $refs #385

Open
ij1 opened this issue May 1, 2019 · 9 comments
Open

LIcon encapsulation breaks $refs #385

ij1 opened this issue May 1, 2019 · 9 comments
Labels
bug confirmed Issue is accepted as either a bug or a valid enhancement help wanted

Comments

@ij1
Copy link
Contributor

ij1 commented May 1, 2019

Description

Steps to Reproduce

  <l-marker
    ref = "marker-container"
    :lat-lng = "[-10, 10]"
  >
    <l-icon
      ref = "icon-container"
      :icon-size = "iconSize"
      :icon-anchor = "iconAnchor"
    >
      <canvas
        ref = "mark-canvas"
        :width = 'iconSize[0]'
        :height = 'iconSize[1]'
        :style = "{
          position: 'absolute',
          left: '0px',
          top: '0px',
        }"
      />
    </l-icon>
  </l-marker>
</template>

Then try to draw into this.$refs['mark-canvas'].

Expected Results

The marker/icon/canvas matches what is drawn to the canvas.

Actual Results

Nothing appears on the canvas. It turns out that there are in fact two canvases, and the visible one is not accessible through $refs as expected (drawing goes to the second, non-visible canvas).

I could access the visible canvas copy with this.$refs['marker-container'].mapObject.getElement().firstChild but is that really the way it is supposed to work
(after waiting long enough, in mounted or in $nextTick even that fails)?

Browsers Affected

  • [ x ] Firefox

Probably others too.

Versions

  • Leaflet: v1.3.4
  • Vue: v2.5.18
  • Vue2Leaflet: v2.1.1
@DonNicoJs
Copy link
Member

@ij1 Wow this is a use case that I did not think would exist/work.

So how the system works is that what you put inside l-marker gets used as a base reference and is content transferred to leaflet with the appropriate methods/class initialisation.

if you manipulate the canvas directly we have absolutely no way to know that the leaflet part needs to be updated.

My suggestion is to: update to the latest version
put a @ready listener on the marker and when is fired you can access the 'leaflet' canvas via the returned mapObject.

Just be wary of performances with lots of canvasses

@ij1
Copy link
Contributor Author

ij1 commented May 2, 2019

@ij1 Wow this is a use case that I did not think would exist/work.

To be honest, I didn't think that either but I was considering that I might have to write my own code for using canvases with marker like objects but then by chance wandered to the l-icon docs page and realized I could just replace the image in the given example with a canvas! And that warranted testing it out :-).

What I've done so far to solve this problem is passing a code-generated svg to the marker icon but the svg boilerplate and general messiness of constructing svg on the fly makes it quite annoying. Also, I don't think that the canvas will be a loser performancewise either compared with all the extra steps svg path goes through.

Basically my use-case requirements are: placement of a reference pixel (usually the center pixel) in latLng, drawing/object sizing in screen coordinates, and ability to update the marker image. The updating is mostly infrequently, up to a few times per minute; except one case where the rotation(s) are aimed by mouse for which svg approach is lagging somewhat (w/o any hw accelerations) but I guess canvas will easily handle without lag. One of the rotations is eccentric making some pre-existing rotatingmarker stuff based on css transformations useless without extra math hassle combined with multiple objects, canvas' rotate+translate+rotate, on the otherhand, neatly handles it without that extra trigonometry in js.

So how the system works is that what you put inside l-marker gets used as a base reference and is content transferred to leaflet with the appropriate methods/class initialisation.

if you manipulate the canvas directly we have absolutely no way to know that the leaflet part needs to be updated.

I realize that. My intention was not to modify a non-visible canvas (nor to suggest its changed should be tracked).

During testing it out I just realized I ended up modifying that "shadow" canvas instead of the one I wanted to as I didn't even know, at that time, the canvas given in the template would not be made visible but just used as blueprints. I would have expected the framework to take that particular instance of canvas and graft it into the marker.

My suggestion is to: update to the latest version
put a @ready listener on the marker and when is fired you can access the 'leaflet' canvas via the returned mapObject.

Yes, I realized from the git log I can probably use that for the timing. It will of course leave those dummy canvases into dom but hopefully they won't have any impact.

Anyway, thanks for the suggestion.

Just be wary of performances with lots of canvasses

I know this is somewhat off-topic but I've been trying to search for information about this but so far with little success, do you have any references/experience about this? (As I'm also thinking of migrating even some of my gridlayer stuff into small of canvases to avoid zooming changing their scale + redraw to correct it afterwards). Most of the references claiming this seem to talk about large canvases. I'm quite surprised that I've not found anyone to even mention anything along the lines of one canvas per "sprite" or so (drawImage to copy sprites from small non-visible canvases to a large canvas does not count but are easily found though), but it could be just that the search terms are just too generic around this topic to return good results from a google search.

@ij1
Copy link
Contributor Author

ij1 commented May 6, 2019

put a @ready listener on the marker and when is fired you can access the 'leaflet' canvas via the returned mapObject.

This sort of works, I had to still put yet another this.$nextTick into the ready handler be able to acquire the canvas instead of the icon image.

I just tried resizing the icon/canvas + recentering the anchor and it broke again. I guess the icon gets rewritten so that the old canvas is thrown away and the new one is put to DOM. Not a big deal though, I'll just keep the size constant and scale the drawing parameters to work-around it.

@DonNicoJs
Copy link
Member

@ij1 do you use the last version? Also do you think we could do a dedicated component?

@ij1
Copy link
Contributor Author

ij1 commented May 7, 2019

@ij1 do you use the last version?

Yes, I think so:

* Vue2Leaflet: v2.1.1

...so the $nextTick($emit('ready')) stuff should be there already.

Also do you think we could do a dedicated component?

For static content, I guess the current approach is ok. But for truly HTML like content such as DivIcon I think my natural expectation is somewhat more (more "vue-like" handling for the slot content) and anything else ends up into a surprised: "ow, that didn't work". Such surprise, of course, is not the end of the world and in this case I already implemented a way to workaround it. I'm no longer sure if resizing the canvas was that useful anyway (it was just a side-product of scaling the drawing), the constant size approach is likely fine for me, if not even better.

I guess it ultimately boils down into the difference how Leaflet itself handles (Div)Icon and Control (LControl puts the slot content into DOM rather than recreating it). In Leaflet, the former can be instanciated many times from a JS variable so the same elements cannot be easily reused and the content is passed in the html option instead (it's intented to be a reusable icon, after all). But with templates in vue there's no need to be similarly prepared for multiple copies so the DivIcon wrapper could put the slot content into DOM similar to how LControl behaves. It would probably need LDivIcon.extend + supplying CreateIcon but not much else?

Looking into LIcon's code now, I also wonder if setIconSize/setIconAnchor really need to recreate the whole icon, I'd think that from Leaflet's point of view, calling _setIconStyles would be enough.
But even with that, I think I still couldn't resize the canvas using width/height attributes (but assigning in JS would work though) as it would trigger setHtml I guess.

@DonNicoJs
Copy link
Member

@ij1 Sadly the approach of putting stuff in the DOM and then feeding it to leaflet is the only one that allows us to still use the reactivity if we don't put the element in the dom it does not get updated.
I am fighting with this since a while but I don't know if we can do better.

About creating only one icon I am thinking to re-write l-default-icon to be a static performant l-icon

_setIconStyles is a private method so we avoid to use them in order to not have a breaking change from the leaflet side that we don't expect ( they are not responsible for internal method changes )

@ij1
Copy link
Contributor Author

ij1 commented May 13, 2019

I'm starting to regret I even made this bug report as I'm afraid my canvas inside divicon trick will break if something gets changed the wrong way :-) (obviously with the no resizing + no $refs restrictions but I can live with those).

I noticed that Leaflet 1.5.0 seems to have added support for giving an element to DivIcon without mucking around with Leaflet's internals:
Leaflet/Leaflet@e079588
I won't help the older versions though (nor me as I'm currently stuck to 1.3.4 because of some gridlayer bug added in 1.4.0 probably related to opacity that makes the whole layer occassionally non-visible).

@DonNicoJs
Copy link
Member

@ij1 No worries we pay attention not to break it :) ALSO! could you provide an example with the canvas technique? It would help us not break anything and also be interesting for others

@stale
Copy link

stale bot commented Jan 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 1, 2020
@DonNicoJs DonNicoJs added bug confirmed Issue is accepted as either a bug or a valid enhancement help wanted and removed wontfix labels Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed Issue is accepted as either a bug or a valid enhancement help wanted
Projects
None yet
Development

No branches or pull requests

2 participants