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

Replace editor-layer-index with @ideditor/imagery-index #7425

Closed
bhousel opened this issue Mar 11, 2020 · 19 comments · Fixed by #7428
Closed

Replace editor-layer-index with @ideditor/imagery-index #7425

bhousel opened this issue Mar 11, 2020 · 19 comments · Fixed by #7428
Assignees
Labels
chore-dependency Improvements to one of iD's dependencies

Comments

@bhousel
Copy link
Member

bhousel commented Mar 11, 2020

We already did have an ELI issue for this (osmlab/editor-layer-index#214, which I mention above as a blocker), and I submitted a PR to fix that and all the other issues with ELI that we have (osmlab/editor-layer-index#490) and nobody wanted it. This was like 2 years ago.

Originally posted by @bhousel in #4994 (comment)

The past 2 weeks I've had some time to dive in and clean up the editor-layer-index project to make it something that works better for us. I reviewed every single source file and rewrote all the code.

Today I released an initial version and I'm feeling good about switching iD over to use it!
The code is here: https://github.com/ideditor/imagery-index

This builds off of the work done in that previous pull request, but adds a bunch more.

✨What's new:

  • Built an awesome preview site - https://ideditor.github.io/imagery-index/ (seriously, check it out)
  • Separated .geojson files from imagery source .json files to promote reuse
  • Leveraged country-coder and location-conflation for defining geographic regions (so we can avoid shipping geojson for boundaries that iD already has)
  • Added many geometry checks to correct geojson winding order, coordinate precision, prettify file formatting
  • Fixed all the GeoJSON MultiPolygons (they were encoded as Polygons before)
  • Used Javascript everywhere for ease of maintenance
  • Published software to npm with semantic version number (We can load these files from a CDN now with guarantee that they will be compatible with iD. We don't have to wait for an iD release to deliver the latest imagery to our users)
  • Included backward-compatible editor-layer-index style files (imagery.json,imagery.geojson,imagery.xml)

Still TODO - I'd like to:

  • Restore the historic imagery and wmts sources, I just skipped these for the initial version to save time (iD does not include old imagery sources).
  • Improve the best flag. We can think of a better way to say which sources are best where.
  • Add a lot more checks to make sure that privacy policies and licenses are set for all the sources. Willing to do this work myself, but would appreciate help. I'd like to work towards making these fields required for all sources.
  • Move all the icons back into the repository so that they are saved somewhere stable, and find better quality version of them (many currently reference files that are 404 or unstable, or .ico files)
  • Produce a better report about how the index differs from JOSM's index

Excited to make the switch 🍾

@grischard
Copy link
Contributor

Note that the license of that project is incompatible with the source you've forked.

@quincylvania quincylvania added the chore-dependency Improvements to one of iD's dependencies label Mar 12, 2020
@bhousel
Copy link
Member Author

bhousel commented Mar 12, 2020

Note that the license of that project is incompatible with the source you've forked.

I don't believe that's true.

It's not a fork - all the files are significantly changed and I went through each one by hand and reworked the geometry. I even put back the imagery license properties that you removed and cleaned up a lot of logos.

We discussed the ELI license a bunch on osmlab/editor-layer-index#490
(can you believe it's been almost 2 years?)

per @stoecker's comment osmlab/editor-layer-index#490 (comment)

If you want ISC in any case drop all data and start from scratch.

That's exactly what I did. My reading of his comment is that the ISC license would prevent some kind of automatic sync with JOSM's wiki, but I'm ok with that.

@thomersch
Copy link
Member

would prevent some kind of automatic sync with JOSM's wiki, but I'm ok with that

Your actions look like they actively want to harm other projects and discourage other people to join discussion or collaboration. We reached out to iD to try to unify the work the OSM community is doing, and you are starting a fork that is probably not legally correct (database law!) and is actively breaking paths for collaboration. Why are doing this?

@stoecker
Copy link

Note that the license of that project is incompatible with the source you've forked.

I don't believe that's true.

It's not a fork - all the files are significantly changed and I went through each one by hand and reworked the geometry. I even put back the imagery license properties that you removed and cleaned up a lot of logos.

It is not SO easy! That would be the same as saying that changing indentation and whitespace in source code files is enough to allow a license change. If you use existing data you need to follow the license of the data. You can't simply copy the data and choose another license. That's clearly a license violation.

@grischard
Copy link
Contributor

If that were possible, we would long ago have reviewed every single proprietary map to make our own :)

@bhousel
Copy link
Member Author

bhousel commented Mar 12, 2020

@thomersch said:

Your actions look like they actively want to harm other projects and discourage other people to join discussion or collaboration.

No that's not true. If you run a project that needs an imagery index, I do want to talk to you about it. (I see it as a crucial dependency for iD).

We reached out to iD to try to unify the work the OSM community is doing, and you are starting a fork that is probably not legally correct (database law!) and is actively breaking paths for collaboration. Why are doing this?

No that's not true either. You and Stereo started buliding (in secret) a replacement for ELI using vector tiles. We told you that that wouldn't work for us. What we really want is something like ELI but with 1. the 3 year old bugs fixed, 2. more efficient workflow, 3. actually published with versions. So I built that.

@bhousel
Copy link
Member Author

bhousel commented Mar 12, 2020

@stoecker said

That would be the same as saying that changing indentation and whitespace in source code files is enough to allow a license change.

No that is not true, you need to change all the code and files much more significantly than that.

You can't simply copy the data and choose another license.

I agree! I didn't do that.

@thomersch
Copy link
Member

thomersch commented Mar 12, 2020

No that's not true. If you run a project that needs an imagery index, I do want to talk to you about it. (I see it as a crucial dependency for iD).

I guess except if the person works on JOSM...?

No that's not true either. You and Stereo started buliding (in secret) a replacement for ELI using vector tiles. We told you that that wouldn't work for us. What we really want is something like ELI but with 1. the 3 year old bugs fixed, 2. more efficient workflow, 3. actually published with versions. So I built that.

You didn't talk to me one word, but instead tried to lead a secret conversation. We talked to Quincy which you intimidated so much that he didn't even want to talk to us anymore.

My only remaining questions are: Is this a Mapbox project? Is imagery index owned by Mapbox?

@grischard
Copy link
Contributor

grischard commented Mar 12, 2020

For the record: it's been public at osmlab/editor-layer-index#586 for more than a year, and I contacted you to ask what changes would be needed to make it work for you.

I regret that you chose to do your own hard fork and not automatically base this off an existing index. This will deepen the rift between the two or three imagery databases, provide a worse experience for mappers and waste volunteer time required to add imagery to all editors.

I'm also concerned that this will be interpreted as a middle finger by the community, when we should be finding more ways of building trust and of working together. How do you think the ELI maintainers and Thomas are feeling right now?

@bhousel
Copy link
Member Author

bhousel commented Mar 12, 2020

You didn't talk to me one word, but instead tried to lead a secret conversation. We talked to Quincy which you intimidated so much that he didn't even want to talk to us anymore.

That's.. also not true. 🙄
(we can do this all day I guess)

My only remaining questions are: Is this a Mapbox project? Is imagery index owned by Mapbox?

No. I think it's fair to say that Mapbox could not care less about any of this.

@quincylvania
Copy link
Collaborator

We talked to Quincy which you intimidated so much that he didn't even want to talk to us anymore.

Hi @thomersch! I'm not sure how you came away with this impression, Bryan never intimidates me about anything 😂. I said I was leaving the conversation because it was exhausting sitting in the middle of the argument.

I'm excited that Bryan built the index we need to meet iD's technical goals and I regret that ELI wasn't flexible enough to meet them itself.

@bhousel bhousel mentioned this issue Mar 12, 2020
@quincylvania quincylvania added this to the Next Release milestone Mar 13, 2020
@matkoniecz
Copy link
Contributor

matkoniecz commented Mar 14, 2020

all the files are significantly changed and I went through each one by hand and reworked the geometry

I've had some time to dive in and clean up the editor-layer-index project

So it is based on ELI, with some changes made to it, entry by entry? Making it a derivative work, what requires obeying license of the used materials?

Note that "significantly changed" is not removing copyright. Manual editing is not enough to remove copyright.

regret that ELI wasn't flexible enough to meet them itself.

Have you tried reporting that kind of functionality/data was missing and that it is necessary for iD (note earlier "I contacted you to ask what changes would be needed to make it work for you.").

@bhousel
Copy link
Member Author

bhousel commented Mar 14, 2020

So it is based on ELI,

Not really, imagery-index is actually based on the osm-community-index code. (the initial commit even says so). This is why I used ISC, because that other project is ISC.

I don't actually have a strong preference, and I find arguments about software licensing tedious - so I'll just switch it to CC-BY-SA to be like ELI if that will make people happy.

Have you tried reporting that kind of functionality/data was missing and that it is necessary for iD (note earlier "I contacted you to ask what changes would be needed to make it work for you.").

Yes many times. Let's review..

I've been putting off this rewrite since like 2016, see
osmlab/editor-layer-index#168
osmlab/editor-layer-index#190
People forget that I was the maintainer of ELI for years. I feel bad that I didn't make it a bigger priority back in 2016.

I even submitted a pull request in 2017 see
osmlab/editor-layer-index#490.
Technically I don't have to submit pull requests to a project that I'm a maintainer of, but this is a nice thing to do when other softwares depend on it. There wasn't interest in the upgrade then so I dropped the issue.

In the summer of 2018, I chatted a bunch with @grischard at State of the Map in Milan and reiterated our need to get those old bugs fixed and actually release software with version numbers.

By late 2018 it was becoming a real problem. A lot of people want iD to use resources that are released on a different schedule from the iD code. It is a real problem to wait for an iD release to push the latest imagery out to our users.:
#4994
#5412
osmlab/editor-layer-index#574

But we can't do that unless all the software we depend on follows a predictable release process with semantic version numbers. The files we fetch must be compatible with the code we've written to process them.

ELI shouldn't just add or remove properties from their distributed files on a whim. Even as recent as last week, ELI still has issues where the robot script deleted files because someone accepted a PR too quickly

Stuff like that can break iD for thousands of users and impact scheduled events like mapathons. So I stand by my decision to not fetch the current imagery.json file from the main branch of the ELI repository and this is why I am being so insistent about following a release checklist, writing a changelog, and publishing software with semantic version numbers.

I understand that switching from one imagery index to a different one can seem a big change, but this is something that we've wanted to do for a very long time, and it unblocks us being able to deliver faster imagery updates to our users without breakage.

@don-vip
Copy link

don-vip commented Mar 14, 2020

Stuff like that can break iD for thousands of users and impact scheduled events like mapathons.
...
it unblocks us being able to deliver faster imagery updates to our users without breakage.

You always justify your decision with this argument, but will you actively monitor imagery entries for health check? Things break all the time, everyday, everywhere, see the JOSM imagery integration test. There are almost every time around 5% of entries broken because of bad TLS certificates, URL changes, temporary failures and so on. You can't avoid imagery to be broken for users.

@bhousel
Copy link
Member Author

bhousel commented Mar 14, 2020

You always justify your decision with this argument, but will you actively monitor imagery entries for health check? Things break all the time, everyday, everywhere, see the JOSM imagery integration test. There are almost every time around 5% of entries broken because of bad TLS certificates, URL changes, temporary failures and so on. You can't avoid imagery to be broken for users.

Thanks @don-vip - this is a fair point.. I understand that imagery itself may go down and that's outside our control. But when users start iD, we need an imagery index to be available.

This was never an issue up until yesterday because previously, we just included ELI with the iD source code. Unbundling it means that we absolutely must fetch a compatible version at iD startup - or iD won't work at all. The file can't be unavailable or corrupted, or have missing properties, or extra properties. Fetching the needed files from the jsdelivr CDN meets these guarantees for us.

Literal months worth of work led up to being able to change these 2 lines of code. iD's builds are now much faster and smaller, iD startup time is faster, and we can get our users the latest imagery, community index, translations, without releasing new iD versions. (next, let's do the presets)

I can't overstate how huge this change is, and how important a milestone it is on our v3 roadmap.

@thomersch
Copy link
Member

Maybe there is some underlying cultural disconnect here, what do sentences like

I regret that ELI wasn't flexible enough to meet them itself.

I can't overstate how huge this change is, and how important a milestone it is on our v3 roadmap.

even mean?

I am under the impression that you're trying to become independent of everyone (v3's direction seems to be a symptom of that) and apparently everyone who is of the impression that such actions can have unintended effects is a naysayer.

I have been always defending iD and still think it is a great thing for a lot of people, but your governance and telling others that you're tired of speaking with us and sending us belittling emoji (looking at you @quincylvania) makes me uncomfortable. Mapbox does not seem to "care", but seems to pay the bill, which is a perfect alibi to distance yourself from any responsibility.

Bryan, you are framing any of your efforts to be the only possible way of improving iD. While I applaud your consistent work, I don't believe this is fair communication. Maybe you do have more professional development experience, but in my feeling collaboration always helps and there is always more than one way of doing it right.

@bhousel
Copy link
Member Author

bhousel commented Mar 15, 2020

@thomersch , I can't be "tired of speaking with" someone I've never spoken to before. I've never met, emailed, or spoken to you, and you've never commented before on any issue in the iD or ELI issue trackers until this one.

If you need an imagery index for something, or if you have some problem with how iD works, sure let's talk about that and I'll try to avoid any "belittling emojis", but please let's just try to communicate honestly and openly.

@thomersch
Copy link
Member

I can't be "tired of speaking with" someone I've never spoken to before. I've never met, emailed, or spoken to you, and you've never commented before on any issue in the iD or ELI issue trackers until this one.

It's not all about you, Bryan. I explicitly said it was about what Quincy said. You chose to speak with Guillaume in private instead of including me in the email conversation we tried to establish, which I am fine with, but please keep it straight yourself.

@quincylvania
Copy link
Collaborator

@thomersch I apologize if my emoji usage came across as belittling! I certainly didn't mean it that way and I didn't consider that possible interpretation. There's probably a cultural disconnect here, since I frequently use and see emoji in a broad variety of online interactions without issue.

With the laughing face I meant to convey a lighthearted tone. Asynchronous online discussions like this often become far pricklier than face-to-face interactions, in-part because nuance is lost in text alone. Emoji are sometimes helpful here, but I guess not in this case.

@openstreetmap openstreetmap locked and limited conversation to collaborators Mar 20, 2020
@quincylvania quincylvania removed this from the Next Release milestone Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
chore-dependency Improvements to one of iD's dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants