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

Various possible features, to-dos, and bugfixes to be broken out #87

Open
3 of 4 tasks
jywarren opened this issue Feb 23, 2018 · 11 comments
Open
3 of 4 tasks

Various possible features, to-dos, and bugfixes to be broken out #87

jywarren opened this issue Feb 23, 2018 · 11 comments

Comments

@jywarren
Copy link
Member

jywarren commented Feb 23, 2018

...into separate issues

(copied in from README)

To do:

  • shift-drag (scale with no rotate) doesnt work if you shift first, only if you drag first
  • default to order by size -- maybe need a custom $L.customOrdering boolean?
  • plenty of other issues

Lower priority:

  • decide if we need to keep updating _bounds - yes
  • create img.toGeoJSON() so we can send a concise description of an image to MapKnitter, plus properties:
    • locked
    • layer order
    • last touched/edited?
    • "{"type":"Feature","properties":{},"geometry":{"type":"Polygon","coordinates":[[[-0.08,51.509],[-0.06,51.503],[-0.047,51.51],[-0.08,51.509]]]}}"
  • add onLock, onUnlock, onDistortEnd - and consider plumbing events properly
  • add image ordering -- bringToFront() should be temporary only; we need img.raise() or img.lower() or img.raiseToTop() etc... also img.order() for current position in order
  • add an img.revert() which reverts it to orig dimensions and rotation
  • implement tab to select next image; $L.selectedIndex?

=================

Leftovers, persnickity stuff:

  • plumb or remove debug system
  • make shift-drag drag the nearest marker, not the image?
  • scale is not true scaling -- it moves points equally from the "center" which causes distortion when scaling down a lot
@rexagod
Copy link
Member

rexagod commented Jul 7, 2019

@sashadev-sky Can you please glance over the issues above and let me know if you've started/finished up on anyone of these in any of your PRs? That'll be really helpful to know. I can then start working with the rest of these.

Some of these I think are done already:

  • add image ordering -- bringToFront() should be temporary only; we need img.raise() or img.lower() or img.raiseToTop() etc... also img.order() for current position in order
  • create img.toGeoJSON() so we can send a concise description of an image to MapKnitter, plus properties:
  • Any other issue that comes to mind?

@jywarren
Copy link
Member Author

jywarren commented Jul 8, 2019 via email

@sashadev-sky
Copy link
Member

@rexagod Looking them over! @jywarren would appreciate your input on these points:

  • shift-drag (scale with no rotate) doesnt work if you shift first, only if you drag first

    • shift-drag is BoxSelector! BoxSelector doesn't display this problem. Why did we want to have this for scale? Pretty sure this is old?
  • decide if we need to keep updating _bounds

    • also old because we use corners instead of _bounds? But I think starting to track _bounds is something we want to start doing because a lot of Leaflet functions work on _bounds and it'll be easier to debug some current issues we're having. Perhaps open an issue to create a utility function that converts corners to a bounds and vice versa?
  • add onLock, onUnlock, onDistortEnd - and consider plumbing events properly

  • add an img.revert() which reverts it to orig dimensions and rotation

    • this is exactly what _restore does. but it would be called img.editing._restore. Do you want it exposed as public API (restore), testing, etc.? I can open an issue to do that! I can also rename it "revert"! That actually makes a bit more sense than "restore".
  • make shift-drag drag the nearest marker, not the image?

    • same feedback as the first point in this list
  • add image ordering -- bringToFront() should be temporary only; we need img.raise() or img.lower() or img.raiseToTop() etc... also img.order() for current position in order

    • we are currently still using bringToFront()? Did you address this in one of your open PRs?
  • create img.toGeoJSON() so we can send a concise description of an image to MapKnitter, plus properties:

    • not sure about this one but I think this is veery different from what we achieved in generateExportJson.

@jywarren
Copy link
Member Author

jywarren commented Jul 9, 2019

shift-drag (scale with no rotate) doesnt work if you shift first, only if you drag first

I think this is OK. This comes up sometimes when people want to ONLY scale something, but preserve its rotation. we should be able to detect clicking on a handle and not BoxSelect if so, and get this working in either order, but not a priority!

_bounds -- yeah, agree with a lot of Leaflet functions work on _bounds -- for sure!

+1 revert as the new restore!

toGeoJson is not that important either. In theory we might've chosen GeoJSON as a standard that we could build generateExportJSON around, but we didn't, and it's not a big deal. Maybe as part of a future breaking change? someday? 😄

@jywarren
Copy link
Member Author

jywarren commented Jul 9, 2019

I think highest priorites now are:

  • add spinner to multi-exporter
  • submenus

Then:

@sashadev-sky
Copy link
Member

@jywarren I don't remember shift + drag for scaling ever being a feature! I could add this, but it doesn't seem like a convenient way to scale from the keyboard, having to target the handle exactly.

Do you think maybe we should see through #165 (rotate gesture support) first and then decide what makes sense after? I am not sure if I will end up adding scaling to it (hopefully will) and not sure if it will just be touch or also mousepad (have to double check, so far I think it is leaning towards touch only).

@sashadev-sky
Copy link
Member

@jywarren geoJSON may help solve some bugs. I am slowly trying to root out the primary library bugs right now but I think they will involve some big changes - from switching to webGL or Canvas, SVG, geoJSON, or using only transformation matrices. Still a lot to think about there!

gesture rotation was working okay last time I worked on it. Still needs cleaning up and have to verify the implementation details. But it cannot use css transformations, and using _rotateBy and _scaleBy is buggy (probably even more buggy because touch gestures are less precise). So this is why i'm trying to work out all the kinks before. I will make sure, though, to fix it up and prepare for merge soon whether or not I resolve the issues.

@jywarren
Copy link
Member Author

jywarren commented Jul 10, 2019 via email

@sashadev-sky
Copy link
Member

@jywarren I just got the initial placement and animateZoom of the images to work with transform matrices in #339 so v happy about that! Hoping I can get rotation and scale to work with it, and then the distortion will be gone 🎉

@sashadev-sky sashadev-sky mentioned this issue Jul 15, 2019
5 tasks
@7malikk
Copy link
Collaborator

7malikk commented Oct 13, 2022

@jywarren Hello there, I'm an outreachy applicant, I'm interested in contributing to the Mapknitter project.
I noticed some tasks in this issue have not been checked off. Can I convert them to FTOs or work on them myself?

@7malikk
Copy link
Collaborator

7malikk commented Oct 13, 2022

@TildaDares Hello again, I noticed some tasks in this issue have not been checked off. Can I convert them to FTOs or work on them myself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants