Skip to content

Conversation

@haider8645
Copy link

Hello @planthaber,

I had to make the TravNode.hpp available over a rock port and it needs the Angle.hpp to be serialized.

Best,
Haider

@chhtz
Copy link
Member

chhtz commented Apr 9, 2025

Not sure if we need to pull boost-serialization into this. Probably an out-of-class definition inside TravNode.hpp would be a less intrusive option?

But I don't strictly oppose adding it here.

@chhtz
Copy link
Member

chhtz commented Apr 9, 2025

Unrelated: Why does AngleSegment have a width a start and an end? The end appears to be calculated always in the constructor and I doubt that caching an addition result is worth adding a member variable ...

@haider8645
Copy link
Author

Not sure if we need to pull boost-serialization into this. Probably an out-of-class definition inside TravNode.hpp would be a less intrusive option?

But I don't strictly oppose adding it here.

Thanks, I'll do that.

@haider8645
Copy link
Author

@chhtz the out of class definiton works. Thanks!

I'll close this MR, alright?

@haider8645
Copy link
Author

Unrelated: Why does AngleSegment have a width a start and an end? The end appears to be calculated always in the constructor and I doubt that caching an addition result is worth adding a member variable ...

There is a function getEnd() and endRad is used in various computations. Isn't it worth to have it saved as a member rather than always computing from start and width?

@chhtz
Copy link
Member

chhtz commented May 12, 2025

There is a function getEnd() and endRad is used in various computations. Isn't it worth to have it saved as a member rather than always computing from start and width?

I kind of doubt that saving one addition is worth storing redundant data. Moreover, it is easy to create an ill-formed representation, by modifying width without endRad. If computing the sum is time-critical, users could cache the result of getEnd().

But this should probably be discussed elsewhere.

@chhtz chhtz closed this May 12, 2025
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