-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
fix: merge line if it needs to be simplified #244
Conversation
This ensure we still simplify passed line to `mergeLineStrings` even if there is only one
https://github.com/onthegomap/planetiler/actions/runs/2474370787 ℹ️ Base Logs f93e522
ℹ️ This Branch Logs 7479ec9
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is pretty subtle, would it be possible to add a test to verify so I retain this behavior when I refactor later?
@msbarry i will but only in a few days/week. sorry |
Added a test for this, but I'm not 100% sure why it's needed - all features get simplified during the initial processing phase so if just 1 feature ends up in a group then it should already be simplified. Are you simplifying with a smaller tolerance during initial processing, and larger during postprocessing? No rush on an answer, I'll target this for next release so it's not holding anything up. |
@msbarry maybe i understood wrong. But what i understand for when we use
So if this is true we can get into I think this is why you did the same for |
You can opt-out of simplification or min size limits in process, but in practice we still simplify, but only opt-out of the length limit if we're going to merge in postProcess. So this will only make a difference if you set simplify tolerance to 0 in process. |
@msbarry yes this i agree. isn't it what we do for all layers where we merge lines ? |
In the openmaptiles config, it sets the min length to 0 for all line layers that get merged in postProcess, but process also simplifies them like normal so this code would have no effect on the output, just add processing time because we're re-simplifying lines that are already simplified. If there was a use-case for setting the simplification tolerance to 0 in process then this might make sense but I'm not seeing that use-case... |
@msbarry oh if that is so yes no need. Could we see it as a safeguard? Otherwise simply close this PR! |
Added a |
Kudos, SonarCloud Quality Gate passed! |
@msbarry that seems good to me ! thanks |
This ensure we still simplify passed line to
mergeLineStrings
even if there is only one