Skip to content
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

Refactor quadtree: Extracted into tfs::map::quadtree namespace with code improvements and tests #4798

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

ramon-bernardo
Copy link
Contributor

@ramon-bernardo ramon-bernardo commented Oct 5, 2024

Pull Request Prelude

Changes Proposed:

New namespace

  • The Quadtree logic has been extracted from the Map class and relocated to a new namespace: tfs::map::quadtree.

Improved readability

  • Added comprehensive code comments and documentation to clarify the purpose and behavior of several functions, including:
    • find_in_range(...)
    • move_creature(...)
    • create_tile(...)
  • Additionally, we no longer use inheritance in favor of composition.

Enhanced creature search

  • The find_in_range(...) function now utilizes std::experimental::generator for a more efficient search of creatures within a specified range. This update positions the code for future integration with std::generator upon the adoption of C++23.

Tests

  • Comprehensive tests have been introduced to verify the functionality of:
    • Creature movement
    • Range-based queries within the quadtree

Note: Tests for tile finding and creation are still pending.

Usage

// Define coordinates and create a new tile.
auto x = 3;
auto y = 3;
auto z = 7;
auto tile = new Tile();
tfs::map::quadtree::create_tile(x, y, z, tile); // Create a tile at (3, 3, 7)

// Create a new creature and add it to the quadtree.
auto creature = new Creature();
tfs::map::quadtree::push_creature(x, y, creature); // Push the creature into the quadtree.

// Find creatures within a specified range and process them.
for (const auto& creature : tfs::map::quadtree::find_in_range(0, 0, 5, 5)) {
    // found creature...
}

// Move the creature to a new location.
tfs::map::quadtree::move_creature(x, y, 6, 6, creature); // Nove the creature from (3, 3) to (6, 6).

// Find creatures again after movement.
for (const auto& creature : tfs::map::quadtree::find_in_range(0, 0, 5, 5)) {
    // nothing...
}
for (const auto& creature : tfs::map::quadtree::find_in_range(6, 6, 10, 10)) {
    // found creature...
}

// Remove the creature from its current location.
tfs::map::quadtree::remove_creature(6, 6, creature); // Remove the creature from (6, 6).

// Check for creatures within the range again after removal.
for (const auto& creature : tfs::map::quadtree::find_in_range(0, 0, 10, 10)) {
    // nothing...
}

src/quadtree.cpp Outdated Show resolved Hide resolved
src/quadtree.h Outdated Show resolved Hide resolved
src/quadtree.cpp Outdated Show resolved Hide resolved
src/quadtree.cpp Outdated Show resolved Hide resolved
src/quadtree.cpp Outdated Show resolved Hide resolved
src/quadtree.h Outdated
void move_creature(uint16_t old_x, uint16_t old_y, uint8_t old_z, uint16_t x, uint16_t y, uint8_t z,
Creature* creature);
void insert_creature(uint16_t x, uint16_t y, uint8_t z, Creature* creature);
void remove_creature(uint16_t x, uint16_t y, uint8_t z, Creature* creature);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Position rather than 3 ints for xyz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I used pos, but to maintain compatibility with the Map class, I used x, y, and z. I would prefer if Map were also changed to support only Position. See

forgottenserver/src/map.h

Lines 177 to 190 in 2444655

Tile* getTile(uint16_t x, uint16_t y, uint8_t z) const;
Tile* getTile(const Position& pos) const { return getTile(pos.x, pos.y, pos.z); }
/**
* Set a single tile.
*/
void setTile(uint16_t x, uint16_t y, uint8_t z, Tile* newTile);
void setTile(const Position& pos, Tile* newTile) { setTile(pos.x, pos.y, pos.z, newTile); }
/**
* Removes a single tile.
*/
void removeTile(uint16_t x, uint16_t y, uint8_t z);
void removeTile(const Position& pos) { removeTile(pos.x, pos.y, pos.z); }

@@ -112,6 +112,7 @@
<EnableEnhancedInstructionSet>AdvancedVectorExtensions</EnableEnhancedInstructionSet>
<AdditionalIncludeDirectories>$(VcpkgRoot)include\luajit;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<LanguageStandard>stdcpp20</LanguageStandard>
<PrecompiledHeader>NotUsing</PrecompiledHeader>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this before merge ;)

@ramon-bernardo
Copy link
Contributor Author

We could rename the namespace to namespace tfs::map::quadtree and later change the Map class to namespace tfs::map. I think it's a good move, along with namespace tfs::map::astar.

src/quadtree.cpp Outdated

void tfs::map::quadtree::move_creature(uint16_t old_x, uint16_t old_y, uint16_t x, uint16_t y, Creature* creature)
{
if (auto old_leaf = find_leaf_in_root(old_x, old_y); auto leaf = find_leaf_in_root(x, y)) {

Check failure

Code scanning / CodeQL

Inconsistent nullness check

The result of this call to find_leaf_in_root is not checked for null, but 90% of calls to find_leaf_in_root check for null.
@Zbizu
Copy link
Contributor

Zbizu commented Oct 6, 2024

suggestion: add extra tree layer for Z coordinate in order to support up to 256 floors

this can still be regulated by MAP_MAX_LAYERS, but would allow otc users to break 16 floors limit more easily

@ramon-bernardo ramon-bernardo changed the title Rework quadtree and tests Refactor quadtree: Extracted into tfs::map::quadtree namespace with code improvements and tests Oct 6, 2024
@ramon-bernardo
Copy link
Contributor Author

@Zbizu I didn't understand your suggestion. Are you proposing to have a configuration for MAP_LAYER? If so, we can do that when we move the Map class to a namespace. ;)

@ramon-bernardo
Copy link
Contributor Author

Any suggestions for std::generator?

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.

3 participants