Skip to content

Conversation

afarber
Copy link
Contributor

@afarber afarber commented Aug 30, 2025

Implement warning system for issue #240 to prevent silent data loss, when OSM files contain "locations on ways" that will be lost in the output format.

@afarber afarber force-pushed the 240-warning-lost-ways branch from b56b350 to 00f056b Compare August 30, 2025 10:39
@joto
Copy link
Member

joto commented Aug 30, 2025

Locations on ways are a niche subject. Can you tell me what your use case is why you want these changes?

Also: Location on ways should be detectable from the header of the PBF file, so a simpler check should be possible.

@afarber
Copy link
Contributor Author

afarber commented Aug 30, 2025

Hi Jochen, from your comments on bounding boxes I have thought that headers are not reliable.

Thus my approach is - adding to the existing loops, but I can adapt the PR.

I don't have a use case, just want to contribute to the project.

Do you have any open issues/ideas which I could impement?

@joto
Copy link
Member

joto commented Aug 30, 2025

from your comments on bounding boxes I have thought that headers are not reliable.

Bounding box headers have their issues, but that doesn't necessarily apply to other headers. And littering the whole code with checks isn't worth it for a niche feature like the locations on ways. So in this case I would go by the header.
Which also means it would only work for PBFs, because there is header for that in XML files, but nobody who uses advances features like that would use XML, so that's fine.

Do you have any open issues/ideas which I could impement?

I'll check the issue list and open some more issues if I can think of any. Most issues will probably be in libosmium though.

@joto
Copy link
Member

joto commented Aug 31, 2025

@afarber Every time you are changing something on the PR I'll get notified. Can you please work in a local branch and only push to the PR if you are some place where you want me to have a look at it?

Also: Why are you changing the "first file" stuff? Lets agree on a way to do all of this first, before you implement this on many commands.

@afarber
Copy link
Contributor Author

afarber commented Sep 3, 2025

Yes, please share how would you want this feature implemented.

I have understood that you do not want to introduce too many changes for this minor feature, so I have reworked the PR again to have the changes in src/io.cpp only and not in each command.

The reason I push so often is that I unfortunately do not have a Windows build machine, so I use Github CI workflow.

@afarber afarber marked this pull request as ready for review September 3, 2025 14:49
@joto
Copy link
Member

joto commented Sep 4, 2025

so I have reworked the PR again to have the changes in src/io.cpp only and not in each command.

The code seems still overly complicated, maybe I am missing something. Essentially we need two things: Figure out for all input files if locations_on_ways is set on any of them. And then each command must decide based on that info plus command line settings whether to issue a warning or not. The warning will probably be different, some commands have an option and will work with locations on ways, others will never work with location on ways.

The reason I push so often is that I unfortunately do not have a Windows build machine, so I use Github CI workflow.

Pushing is fine, just not into the PR. If you push into your branch, Github should run the actions in your Github repo. It seems this does not work for your repo for some reason at the moment, maybe you need to enable that in your Github settings. Only when you are at a point where you need me to see it, you'll open a PR (or push into the branch of an existing PR).

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

Successfully merging this pull request may close these issues.

2 participants