Skip to content

Reimplement distance check via ai and optimize performance. - #88

Open
DivineThreepwood wants to merge 6 commits into
devfrom
bugfix/46-distance-map-computation-is-still-a-performance-issue
Open

DivineThreepwood wants to merge 6 commits into
devfrom
bugfix/46-distance-map-computation-is-still-a-performance-issue

Conversation

@DivineThreepwood

Copy link
Copy Markdown
Member

📜 Description

Changes proposed in this pull request:

  • Improve goToMarker and goToSupportAgent performance by
    • choosing a more efficient distance represenation
    • caching distance map if possible
    • optimize locking strategy

Test Procedure

  • Please test if navigation to marker and navigation to support agents is performed as expected.

@tommy33790 tommy33790 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The pipeline didn't compile.
And see comments about smaller unused / potentially unnecessary code.

I'm unsure about that whole Thread.currentThread().isInterrupted-stuff - whether it's useful or unnecessary.

companion object {
const val RASTER_SIZE: Int = 10
private const val DISTANCE_MAP_CACHE_SIZE = 128
private val distanceMapCaches = WeakHashMap<AbstractLevel, DistanceMapCache>()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is DistanceMapCache per level necessary if it's always cleared when starting a game (in AbstractLevel.reset() via clearDistanceMapCache)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We’ll keep the distance-map cache scoped per level, with a maximum of 128 recently used maps per level. Since the game lifecycle allows only one active level at a time, AbstractLevel.reset() will clear the entire cache, rather than removing only that level’s entry. This promptly releases cached maps for inactive levels while preserving level isolation, so distance maps from different layouts cannot be mixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still don't get the reasoning / implementation behind distanceMapCaches. It's a mapping for Level -> DistanceMapCache but it's cleared when starting a level. Thus, it will never contain more than one single Level & DistanceMapCache (which itself has up to 128 cached distances). If you start a game with the same level you just had, it's still cleared and all distances need to be recalculated.
With this implementation it's essentially used to store a single DistanceMapCache (that of the current level). So why is it a HashMap for Level -> DistanceMapCache?

Please think about once again.
After that, if you're still convinced that it's implemented / used correctly, then I'm fine with it.
It's just that I, myself, cannot grasp why it's the way it is.

I don't want to prevent a really good improvement with my cluelessness tho.

?.angle
}

private fun stablePosition(): Boolean =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

stablePosition has no references.

Comment thread src/main/kotlin/org/openbase/planetsudo/level/LevelView.kt
@DivineThreepwood

Copy link
Copy Markdown
Member Author

All requested changes addressed.

@Jodi-Pa

Jodi-Pa commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Currently the agents struggle to decide to which agent they should go, if multiple agents are requesting support.

@Jodi-Pa Jodi-Pa added the tested label Oct 14, 2026

@tommy33790 tommy33790 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking really good.
Just think about my one new comment (/genuine)

And then merge it.

companion object {
const val RASTER_SIZE: Int = 10
private const val DISTANCE_MAP_CACHE_SIZE = 128
private val distanceMapCaches = WeakHashMap<AbstractLevel, DistanceMapCache>()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still don't get the reasoning / implementation behind distanceMapCaches. It's a mapping for Level -> DistanceMapCache but it's cleared when starting a level. Thus, it will never contain more than one single Level & DistanceMapCache (which itself has up to 128 cached distances). If you start a game with the same level you just had, it's still cleared and all distances need to be recalculated.
With this implementation it's essentially used to store a single DistanceMapCache (that of the current level). So why is it a HashMap for Level -> DistanceMapCache?

Please think about once again.
After that, if you're still convinced that it's implemented / used correctly, then I'm fine with it.
It's just that I, myself, cannot grasp why it's the way it is.

I don't want to prevent a really good improvement with my cluelessness tho.

@DivineThreepwood

Copy link
Copy Markdown
Member Author

@Jodi-Pa is

Currently the agents struggle to decide to which agent they should go, if multiple agents are requesting support.

still an issue?

@Jodi-Pa

Jodi-Pa commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

@Jodi-Pa is

Currently the agents struggle to decide to which agent they should go, if multiple agents are requesting support.

still an issue?

I think it is still an issue, @tommy33790 has looked deeper into it, maybe she knows more.

I only managed to capture one, fairly nondescript , video of it today, i will send it to you.

This branch has not been deployed

No deployments
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.

Distance Map computation is still a performance issue.

3 participants