Skip to content

Update polygone center algorithm#6579

Open
NariaReynhard wants to merge 84 commits intostreetcomplete:masterfrom
HugoTHOLLON:update_polygone_center_algorithm
Open

Update polygone center algorithm#6579
NariaReynhard wants to merge 84 commits intostreetcomplete:masterfrom
HugoTHOLLON:update_polygone_center_algorithm

Conversation

@NariaReynhard
Copy link
Copy Markdown
Contributor

@NariaReynhard NariaReynhard commented Oct 20, 2025

As mentioned in #4965 fixed the awkward placement of some of the quest marker in center of polygons. The change is mostly contained in a single folder.

Fixes #4965

NicodeH and others added 30 commits September 3, 2025 11:13
Added and filled the backlog for first sprint
…op of the document

Added a new entry for telework on 18/09/2024.
69 to AddBikeRentalType(), // generally less overlap of possible types/fewer choices/simpler to answer
70 to AddBikeParkingType(), // used by OsmAnd
71 to AddBikeParkingAccess(),
72 to AddBikeParkingFee(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you seem to have unrelated edits and commits

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might have done a mistake while trying to revert some changes made from our group that have nothing to do with this specific PR, I'll look into that to fix the issue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I know what happened.

We did join everything on our master branch at some point. We reorganized our way of working to be able to PR one feature each time, so I cleared my branch manually because it was already opened before we cleared our master.

The thing is, one of our quests got accepted in the meantime, meaning that the PR sees it as if I actively deleted it with intent.

I fixed questmodule.kt already, I'll fetch the missing files and readd them to my PR and we should be good to go

Copy link
Copy Markdown
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still unrelated changes. You can check for yourself (latest) in the "Files Changed" tab. Maybe put your code into a new PR instead of trying to revert all those changes made in that fork-repo?

  • Assuming it is a port (of the java port? of the original?) You should add a comment where you describe from where this was ported.
  • the place where you ported it certainly had test cases, right? These should be ported, too
  • where is the difference between a Point and a LatLon? Do we need the Point?
  • why is the extra data structure Polygon needed? Can't it operate on a List<List<LatLon>>, just like for example List<List<LatLon>>.measuredMultiPolygonArea does? For consistency within the project
  • PriorityQueue: I am not familiar with it. It would be nice if there was a doc comment that explained what it should do. What are you trying to do? poll() for example reads like it returns the first element, but then removes the last element and replaces the first element with it, only to basically bubble-sort that element back to the end of the list?? If I understood that right, it sounds very imperformant!

@NariaReynhard
Copy link
Copy Markdown
Contributor Author

I'm confused on how those appeared, I'm conviced I checked there were no unexpected changes so I don't know how they are here. I'll give it a look to fix all that next week, I'm still way too busy for now to work on that this week.

I'll give a look into everything you mentionned, I admit the maths kinda melted my brain and I don't remember everything I did, I'll give a look into that as well and keep you posted. Thank you for the feedback

@NariaReynhard
Copy link
Copy Markdown
Contributor Author

There are still unrelated changes. You can check for yourself (latest) in the "Files Changed" tab. Maybe put your code into a new PR instead of trying to revert all those changes made in that fork-repo?

[...]

* the place where you ported it certainly had test cases, right? These should be ported, too

* where is the difference between a `Point` and a `LatLon`? Do we need the `Point`?

* why is the extra data structure `Polygon` needed? Can't it operate on a `List<List<LatLon>>`, just like for example `List<List<LatLon>>.measuredMultiPolygonArea` does? For consistency within the project

* PriorityQueue: I am not familiar with it. It would be nice if there was a doc comment that explained what it should do. What are you trying to do? `poll()` for example reads like it returns the first element, but then removes the last element and replaces the first element with it, only to basically bubble-sort that element back to the end of the list?? If I understood that right, it sounds _very_ imperformant!

Okay so I've looked a bit into it and am now able to come back to you.

  • I'll look deeper into tests now and will keep you posted about it.

  • Here, there is no actual difference between a Point and a LatLon. So no, we do not need the Point. However, as you mentionned it would be a good thing to make this as an asset available to whomever it may concern, and in this context, using a Point makes more sense. So I developed it with this in mind, so it will be was easier to export later and have minimal changes here to go from direct implementation to a call of the library.

  • Similar to previous point, I expect to do an update later to leave as many things as possible in the library and restore that consistancy. It's also why I've put everything in a separate folder, to be sure I won't forget anything (I or whoever would like to do it, I'm don't mind sharing that work)

  • I admit I wasn't familiar with the concept before, so I redid some research about it and unless I missed something in my implementation, it should work as intended. The idea is to use a heap, but sort it before pulling datas from it. So you treat the biggest values first. Which what we need in the polylabel algorithm. About it's complexity, I'm not an expert in maths so I can't give you a full detailed evidence about it, but according to what I checked it should be a O(log(n)), so nothing alarming.

If you have anything more on your mind while I look into those tests, feel free to ask me, we're less available than at the start, but we didn't forget nor give up on StreetComplete !

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Marker outside oddly shaped target area

9 participants