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

kinematics: add simple AABB tree for collision checking #298

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

xondika
Copy link
Contributor

@xondika xondika commented Feb 8, 2023

No description provided.

@xondika xondika requested a review from yaqwsx February 8, 2023 14:19
Comment on lines 8 to 9
add_library(aabb include/aabb.hpp)
target_link_libraries(aabb configuration)
Copy link
Member

Choose a reason for hiding this comment

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

Přemýšlím, jestli by nestálo za to založit knihovnu geometry a postupně tam všechny věci stěhovat. A pak mít klidně knihovny jako geometry::aabb (v CMaku). Důvod: začínáme mít spoustu věcí, kterou jsou spíše obecná geometrie a nesouvisí nutně s konfigurací nebo rekonfigurací.

};

// Leaves are expected to be spheres or polygons
template< geometric LeafShape >
Copy link
Member

Choose a reason for hiding this comment

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

+1 za koncept.

Comment on lines 25 to 26
std::optional< LeafShape > shape;
std::array< std::unique_ptr< AABB_Node >, 2 > children;
Copy link
Member

Choose a reason for hiding this comment

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

Nebylo by idiomatičtější mít AABB_InnerNode a AABB_Leaf a children si držet jako kontejner a std::variant< AABB_InnerNode, AABB_Leaf? Proč si to myslím: Vnitřní uzly mají jiné metody než listy. A tedy volat leftChild a rightChild na uzlu, který je listem je nedefinované chování. Když budu mít variant, budu ho muset visitovat a tedy můžu mít čistší rozhraní pro dva typy uzů.

bound = bounding_box( children[ 0 ]->bound, children[ 1 ]->bound );
}

size_t objects() const {
Copy link
Member

Choose a reason for hiding this comment

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

Myslím, že pojmenovat size by bylo výstižnější (a kompatibilní se standardními konstruktory).

Comment on lines 62 to 74
void print( const std::string& prefix = "" ){
if( children[ 1 ] ){
children[ 1 ]->print( prefix + " " );
}
if( shape ){
std::cout << prefix << "o\n";
} else {
std::cout << prefix << "B\n";
}
if( children[ 0 ] ){
children[ 0 ]->print( prefix + " " );
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Obecně nejsem příznivcem "debugovacích metod" na třídách. Jedna jsou ad-hoc, zanáší závislost na streamech (stream ani není konfigurovatelný v tomto případě) a std::string.

Lepší řešení mi přijde zavést buď friend toString anebo přidat rozhraní pro vizitory (které vidí vnitřní strukturu stromu) a napsat "debugovací" funkce pomocí nich (mimo hlavní hlavičku).


using namespace rofi::configuration::matrices;

struct Box {
Copy link
Member

Choose a reason for hiding this comment

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

Nechceš zvážit mít box jako šablonovaný vectorem (něco jako GenericBox) a poskytovat using Box = GenericBox< Vector >? Občas se může hodit mít celočíselný strom. Stejně tak pro ostatní objekty.

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