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

Skip unnecessary processing and other handling of zipping the output #40

Closed
wants to merge 4 commits into from

Conversation

Ebe66
Copy link
Contributor

@Ebe66 Ebe66 commented Oct 17, 2021

See title and changelog.

@mweirauch
Copy link
Contributor

Are you sure about the ".v12" suffix. I am pretty sure it should only be ".12" (matching the current version). At least that is what I see on the device and which also makes that tile verification (for map and routing tiles) succeed in the Companion app.

@Ebe66
Copy link
Contributor Author

Ebe66 commented Oct 17, 2021

Great catch Michael. One downside for me, I have to redo and re-upload the whole planet almost to the hidrive...

@treee111
Copy link
Owner

treee111 commented Oct 18, 2021

wow, this is a massive amount of features and checks implemented. Thanks @Ebe66! 👍

Somewhere, there is a failure under macOS when processing countries throught this branch with all the integrated changes.

I would aim for splitting the changes into several, smaller branches / PR's.
Smaller PR's can be tested & reviewed more straight forward. And when the CHANGELOG gets created automatically in some days in the future, it is also a convenient to have more or less precisely named PR's in place. Hope, this is ok for you!

I extracted changes that belong together into separate PR's:

happy reviewing! ;-)

@treee111
Copy link
Owner

Hi @Ebe66,
thanks for the review of the linked PR's! They are already merged.

Now I have prepared two more, which have most of the functionality and speed-boost I think.
As these are big changes it would be nice if you could have a look if the changes are consistent:

After merging these two I compare your branch with develop and look out for changes I didn't catch until then.
Thanks you very much in advance! :-)

treee111 added a commit that referenced this pull request Oct 22, 2021
…. Use 'v12' tag (keep) filters (#46)

- In the filter stage, do not re-convert the result file(s) back to .osm.pbf format. This saves some time here but more importantly osmconvert is much faster in cutting tiles out of o5m files then .osm.pbf files.
- Switched to using 'v12' tag (keep) filters. This only keeps the tags used by Wahoo's devices render themes. Additionally, filter out all names tags except for those used, like city names. So highway names for example or filtered out.
- At the start of the splitting phase check for the existence of a merged .osm.pbf file. This prevents unnecessary splitting tiles out of for instance Europe when the tile is already processed from a smaller, so faster, part like Germany.

* takeover changes from #40

* osmium does not support writing to o5m format

* Introduce Pull Request Template

* [FEATURE] Add check for required input parameter for CLI and GUI (#41)

- now not only for GUI but also for CLI the country argument is checked
- added a check at the start to see if a country is to process is specified. Otherwise closing the GUI without selecting anything resulted in a runtime error.

* [FEATURE] Enhance check for existing (already downloaded) polygons and .osm.pbf files (#43)

- use the files modification date, not the creation time. On windows this seemed to occasionally see newly downloaded files as the ones they replaced.
- check for a full match instead of doing a wildcard match while checking for existing maps. This prevents matching to multiple maps like australia and australia-oceania.

* refactoring (pylint, formatting, comments)

- change pylint findings
- format constans on save
- delete unused comments

* don't really know what these lines are doing
@treee111
Copy link
Owner

Hi @Ebe66,
i compared the develop- branch against your branch Ebe66:skip-unnecessary-processing.
The last changes are captured in a last PR which is ready to review:

After #55 is merged I would close this PR with a big thanks! 👍

@treee111 treee111 closed this in #55 Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants