maliput / maliput_sparse

A maliput backed capable of modeling RoadNetworks from waypoints.
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Avoid using shared_ptr for the linestring's kdtree #54

Open francocipollone opened 1 year ago

francocipollone commented 1 year ago

See https://github.com/maliput/maliput_sparse/pull/52#discussion_r1100765058 discussion

Summary

https://github.com/maliput/maliput_sparse/blob/471c19ddd0913522813569d835dc5b569d25ab14/include/maliput_sparse/geometry/line_string.h#L274

To keep in mind(from comments):

Then, it makes sense to have a view-type of the LineString that allows you to access it but not necessarily own the memory. It could be arranged somewhere else. For that to happen, we can change the builder to construct LineStrings at the Segment level, and then organize how we want them to be used. That means, we could derive LaneGeometries from the set of LineStrings or explicitly organize the pair of LineStrings at the moment of construction the LaneGeometry. This does not require a shared_ptr.

francocipollone commented 1 year ago

Continuing the conversation from https://github.com/maliput/maliput_sparse/pull/52#discussion_r1098759204

I think it will play against us any potential parallelization we would like to use.

I don't think it will be a blocker for parallelization.


I still believe that using a shared_ptr could be beneficial memory-wise and it will be thread-safe for the LineString.

Let me propose this use case:

With this context, we could make use of the adjacency property and reuse the same linestring, e.g: left linestring of lane 1 == right linestring of lane 2. As the underlying kdtree is under a shared_ptr, this line string will be copyable and the kdtree won't be necessary to be re-created: Better for timing, better for memory usage.

If we want to have this kind of memory management without using shared_ptr we should let the linestring to be owned by other entity (-> segment?) and correctly pass the raw pointer around. Also, the LineString is a pretty basic entity which is expected to be first instantiated by the parser and at that point a figure of "segment" won't be available until the lanes are identified into segments. So it isn't direct how to group the linestrings. From a user point of view, the use of LineString class is way less user-friendly with this last approach.

wdyt @agalbachicar