-
Notifications
You must be signed in to change notification settings - Fork 1
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
Spatial conclusion #63
Conversation
Currently the temp entities uses valid IDs and will wast them.
…riple struct. This will close #33
…DFVector. Also the RDFVector could now be temporary.
…atialIndex for different reference systems possible
…ry pair as reference
…s, contains and within conditions.
…ry pair as reference
…s, contains and within conditions.
…ersion and log infos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, a lot has happened since I last looked into sempr. 😅
There are some things that definitely should be addressed:
- The whole point of GeometricObject is not yet clear to me
- registerProperty has been abused by the GeometricObject beyond all good and evil
- The strange behaviour of findEnvelope you mentioned should be investigated
- The geometry checks in the SpatialIndex might expose unintuitive/unwanted behaviour in the 3D case (because the "more precise" geometry checks are 2D)
Plus some minor, less important remarks, but those should be looked into as well. :)
Nevertheless, thanks for all the contributions, and sorry for the late review!
# undef RDFS | ||
} | ||
namespace rdfs { | ||
const std::string& baseURI(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this line? const std::string& baseURI();
is already declared through the macro below:
#define RDFS(name) const std::string& (name)();
RDFS(baseURI)
Same goes for owl.
|
||
|
||
#pragma db object | ||
class GeometricObject : public SemanticEntity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments would be nice. What is the purpose of this class?
const std::string type() const { return type_; } | ||
void type(const std::string& type); | ||
|
||
void registerProperty(const std::string& predicate, const std::string& object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A method of the same name (but templated) already exists in SemanticEntity
, which you derive from here. Is this on purpose? Or coincidental?
template <class T> void registerProperty(const std::string& predicate, T& property) { /* ... */ }
// ... and derivations ...
These methods are used to register member variables and make them visible in the rdf.
virtual ~RDFVector(){} | ||
|
||
void getTriples(std::vector<Triple>& triples) const; | ||
const Triple& getTripleAt(const size_t index); | ||
bool addTriple(const Triple& triple); | ||
bool addTriple(const Triple& triple, bool replace = false); // if replace than an equal triple will be removed before, otherwise there a multiple entries possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of "replacing" an equivalent Triple? If there's a way to see a difference they were not equal. So replace = true
would be the same as allow_duplicates = false
. The other way round is still intuitive for allow_duplicates
, but replace = false
is ambiguous: What happens with an equivalent triple? Is it just discarded without "replacing" the old one, and there is still only one of present in the vector? Or is it added as a duplicate?
tl;dr: I'd suggest renaming bool replace
to bool allow_duplicates
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe the default should be to not allow duplicates? I don't see any use case for adding the same fact multiple times.
@@ -48,11 +43,15 @@ class RDFVector : public RDFEntity { | |||
TripleIteratorWrapper begin() const override; | |||
TripleIteratorWrapper end() const override; | |||
|
|||
bool validity(const Triple& triple) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this check? Does it depend on the RDFVector, or should this be moved to the Triple-class?
for(auto t : *this) | ||
{ | ||
// empty predicate will add all related object to the result list | ||
if ( predicate.empty() || t.predicate == predicate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't really understand what this is meant to do. 🤔
You have triples with an empty predicate stored in this? Why?
{ | ||
Triple t = triple; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the temporary object & copy here?
return hasSubject_ == other.hasSubject_ && | ||
subject_ == other.subject_ && | ||
predicate_ == other.predicate_ && | ||
object() == other.object() ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operator only checks if the subjects and predicates equal and the objects currently have the same value. To really check if the RegisteredPropertys are equal (i.e., refer to the same member variable) you would need to check the address of the T& value_
in the derived class. But that's obviously not so easy. And I still think this is born from the desire to misuse the SemanticEntity. 😉
ask(spatial); | ||
entity::PointCloud::Ptr entity = entity::PointCloud::Ptr(new entity::PointCloud()); | ||
|
||
for(auto& c : spatial->results) | ||
{ | ||
if(c->discriminator() == "sempr::entity::PointCloud") | ||
if(c->discriminator() == "sempr::entity::PointCloud") // could this be done by the dynmaic cast? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nicer version would be to not hardcode the discriminator, but use the odb type traits:
if (c->discriminator() == odb::object_traits_impl<sempr::entity::PointCloud, odb::id_common>::info.discriminator)
should work. (Though this is still a check for exactly PointClouds. This does not match any derived entities. Just sayin').
@@ -85,7 +85,7 @@ int main(int argc, char** args) | |||
DebugModule::Ptr debug(new DebugModule()); | |||
ActiveObjectStore::Ptr active( new ActiveObjectStore() ); | |||
SopranoModule::Ptr semantic( new SopranoModule() ); | |||
SpatialIndex::Ptr spatial( new SpatialIndex() ); | |||
SpatialIndex3D::Ptr spatial( new SpatialIndex3D(std::make_shared<LocalCS>()) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this work? I thought entities need to be located relative to the coordinate system given to the index, in order for the index to work?
More than stale. (Development proceeded in an internal git). |
Changes: