Skip to content

Conversation

@dudantas
Copy link
Collaborator

@dudantas dudantas commented Sep 27, 2025

Improves performance and correctness of spectator retrieval in Map and Tile by tracking creature ranges and providing efficient access methods. Refactors Thing API for safer null handling, updates the Lua binding for const member functions, and adds GTest-based unit tests for spectator logic. CMake and vcpkg have been updated to support test builds and the Google Test (GTest) dependency.

image

@libergod
Copy link
Collaborator

libergod commented Oct 3, 2025

@dudantas Ready to submit?
image

@dudantas dudantas force-pushed the perf-spectators-map branch from 7eee366 to aea34eb Compare October 4, 2025 18:11
@dudantas
Copy link
Collaborator Author

dudantas commented Oct 4, 2025

@dudantas Ready to submit?

Fixed, test again.

@libergod libergod force-pushed the perf-spectators-map branch from 9b56cd8 to feaeb8b Compare October 4, 2025 22:21
@dudantas dudantas force-pushed the perf-spectators-map branch from feaeb8b to 9b56cd8 Compare October 5, 2025 01:46
@libergod
Copy link
Collaborator

libergod commented Oct 5, 2025

Still having warnings about luainterface

image

Improves performance and correctness of spectator retrieval in Map and Tile by tracking creature ranges and providing efficient access methods. Refactors Thing API for safer null handling, updates Lua binding for const member functions, and adds GTest-based unit tests for spectator logic. CMake and vcpkg updated to support test builds and GTest dependency.
Introduces a new test, RangeFiltering, to verify that getSpectatorsInRangeEx correctly filters spectators based on specified range parameters. The test covers both near and far range scenarios to ensure proper functionality.
Added deduplication of creatures in Map::getSpectatorsInRangeEx and Map::getSpectatorsByPattern using an unordered_set. Updated Tile::appendSpectators to filter by position. Extended tests to verify deterministic ordering, uniqueness, and correct creature span updates on addition and removal.
Added a virtual destructor to ApplicationContext for proper cleanup in derived classes. Updated map_spectators_test.cpp to use try_emplace instead of emplace for m_knownCreatures to avoid overwriting existing entries.
Eliminated the redundant position check for creatures in Tile::appendSpectators, allowing all creatures on the tile to be appended regardless of their position.
Replaces direct calls to stdext::demangle_class with local variables in template methods for class registration and member/static function binding. This improves code readability and maintainability.
Changed type-checking methods (e.g., isItem, isCreature) to be const across Thing and derived classes for better const-correctness. Refactored Map's getSpectatorsInRangeEx and getSpectatorsByPattern to use a helper for cleaning duplicate/new spectators. Updated Thing to make most property accessors const and improved code clarity. Replaced std::reverse with std::ranges::reverse in Tile::getCreatures.
@libergod libergod force-pushed the perf-spectators-map branch from 9b56cd8 to a50f965 Compare October 13, 2025 16:15
@sonarqubecloud
Copy link

Copy link
Collaborator

@libergod libergod left a comment

Choose a reason for hiding this comment

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

Needed to do some changes to fix luabinder warnings, can you check @dudantas ?

Comment on lines +173 to +174
g_logger.warning("Lua warning: member function call skipped because the passed object is nil");
return Ret{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I change this back, luabinder stops giving warnings:
image

Comment on lines +248 to +249
g_logger.warning("Lua warning: member function call skipped because the passed object is nil");
return Ret{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix used in both cases:
image

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.

2 participants