Skip to content

enhance: optimize spawn cleanup removing redundant iteration.#2907

Closed
kaleohanopahala wants to merge 4 commits intoopentibiabr:mainfrom
kaleohanopahala:main
Closed

enhance: optimize spawn cleanup removing redundant iteration.#2907
kaleohanopahala wants to merge 4 commits intoopentibiabr:mainfrom
kaleohanopahala:main

Conversation

@kaleohanopahala
Copy link
Copy Markdown
Contributor

  • Improved performance on large maps by avoiding double iteration.
  • C++20 support.
  • Removed redundant spawn time update.

@Solkrasm
Copy link
Copy Markdown

Me pareceu uma boa, só não sei se retirar o OTSYS_TIME é padrão no OTBR.

Copy link
Copy Markdown
Member

@dudantas dudantas left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I'm curious about the reasoning behind removing the logic that updates the lastSpawn time in spawnMonsterMap. This update is crucial for tracking spawn intervals correctly. Was there a specific reason for omitting it in favor of using std::erase_if? It’s important to ensure that all necessary data remains accurate after cleanup.

Also, please consider using the pull request template provided. It really helps in giving more context and ensuring that all changes are properly reviewed and tested by other developers and contributors.

@kaleohanopahala
Copy link
Copy Markdown
Contributor Author

Hello!
Makes the respawn behavior identical to the original game, without storing redundant times. On larger servers, such as those with original game maps and many monsters being killed, it improves performance by maintaining the original spawn time set in RME/Config.lua, without generating random spawns.

Sorry, I forgot to follow the pull request template.

Copy link
Copy Markdown
Contributor

@luan luan left a comment

Choose a reason for hiding this comment

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

Thanks for the optimization. I didn't quite follow your explanation about lastSpawn (in fact it seems maybe you misunderstood Eduardo's question), but that's ok.

We can remove setting .lastSpawn here because this isn't actually spawning those monsters, it's cleaning up removed ones, setting lastSpawn on removed monsters is not necessary, we set this on SpawnMonster::spawnMonster and that is sufficient.

This code here in fact wasn't accomplishing much other than potentially slowing down spawns of monsters which got removed.

@dudantas dudantas changed the title Optimizing Spawn Cleanup: Removing Redundant Iteration. enhance: optimize spawn cleanup removing redundant iteration. Sep 25, 2024
@dudantas
Copy link
Copy Markdown
Member

dudantas commented Sep 25, 2024

@kaleohanopahala can you open the pull request again? But this time, in another branch? You opened it in your main (fork) and clang-format broke because of that, so I can't merge it
image

@lBaah
Copy link
Copy Markdown

lBaah commented Sep 25, 2024

I`ve tested this change, it makes the spawn time faster than the spawntime defined at spawn.xml.
I recommend not to merge this change yet until we understand the actual spawn time.

Copy link
Copy Markdown
Contributor

@luan luan left a comment

Choose a reason for hiding this comment

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

I`ve tested this change, it makes the spawn time faster than the spawntime defined at spawn.xml. I recommend not to merge this change yet until we understand the actual spawn time.

@lBaah is right. This changes the behavior incorrectly, it makes the next monster spawn based on when it previously spawned, not when it died. I was wrong when I said this didn't do much, it's important and we cant skip the lastSpawn reset.

You can still do this in a single loop though.

@lBaah
Copy link
Copy Markdown

lBaah commented Sep 25, 2024

I agree @luan, we can do it single loop, therefore the proposed change breaks spawn time.

@luanluciano93
Copy link
Copy Markdown
Contributor

luanluciano93 commented Sep 25, 2024

This way it should work correctly

void SpawnMonster::cleanup() {
    std::erase_if(spawnedMonsterMap, [&](const auto &pair) {
        if (pair.second == nullptr || pair.second->isRemoved()) {
            spawnMonsterMap[pair.first].lastSpawn = OTSYS_TIME();
            return true;
        }
        return false;
    });
}

and can also be used in SpawnNpc::cleanup()

@lBaah
Copy link
Copy Markdown

lBaah commented Sep 25, 2024

edited
@luanluciano93 i agree with your changes, therefore i suggest you to make "pair.second" more readable as I find there are abuse of "auto" variable type over the repo

This version improves efficiency by avoiding an intermediate list for removal, saving time and memory. The map is traversed only once, with iterators updated after each removal, making the process more efficient. Additionally, it eliminates redundant lookups by directly modifying the map during iteration.
@kaleohanopahala kaleohanopahala closed this by deleting the head repository Sep 25, 2024
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.

6 participants