Redesign 0.0.2: Introducing Interfaces

[DavidMerzJr]

As described in #147, the current design of noether, especially tool_path_planner and path_sequence_planner, has organically grown over the last few years. Unfortunately, we have reached a point of extensive code duplication between planners and inconsistent planner interfaces. This PR introduces new interfaces which will be used, over the next few months, to consolidate and reorganize the existing functionality of Noether while simplifying expansion in the future.

This PR introduces the new interfaces in a separate package, noether_tpp. It also includes documentation relative to the new design and diagrams describing certain concepts key to planner standardization. Once this PR is merged, @marip8 and I will begin separately migrating pieces of functionality into this package using separate pull requests.

A key goal of this redesign is an experiment in backward-compatibility, so we will be aiming to provide minimum disruption to those currently using the old interfaces. This exercise in transparent changes and deprecation is why we are electing to primarily work through PRs against master, rather than an extended feature branch.

[marip8]

@jdlangs can you take a look at this as well?

[DavidMerzJr]

@marip8 Is CMake 3.10 actually required for this, or would CMake 3.5.1 do? CI is failing on Kinetic/16.04 because of the CMake requirement.

[marip8]

I think 3.5.1 should be fine for this

[marip8]

I'm not a fan of creating several different libraries that users have to depend on separately. This is not that much code overall so what is the argument against having it all bundled in a single library?

It's like this mainly because of the sub-directory structure of the package. We've gone back and forth on this layout a few times, but to me the benefits of the sub-directories are to:

• separate the definition of the interfaces from their implementations (e.g. core vs. tool_path_modifiers)
• separate the implementations of different interfaces from one another (e.g. tool_path_planners vs. tool_path_modifiers)
• smaller self-contained CMakeLists

I wouldn't mind having a single library for this package. Is that possible with the sub-directory approach or would we need to change?

Maybe consider using noether for the namespaces and include paths. If the extra _tpp is there just to avoid collisions it would be nice not to have it permanently down the road.

Agreed. I'll push a change in this PR. I need to update the target namespaces as well

For passing unique_ptrs when constructing objects I don't think r-value references should be used. I believe you should simply pass by value and the compiler will enforce that the user isn't copying it.

I'll try that out. I thought the && was required for unique_ptrs but if the compiler can handle pass by value then I'll make the change

[marip8]

@jdlangs I was able to build a single library from the different sub-directories. Let me know what you think of the approach

[DavidMerzJr]

Maybe too little too late here, as we probably don't want to go back and forth a bunch. However, I had a few comments.

I'm not a fan of creating several different libraries that users have to depend on separately. This is not that much code overall so what is the argument against having it all bundled in a single library?

There isn't much code in here yet, but we are planning on moving most of tool_path_planner, path_sequence_planner, and possibly mesh_segmenter all into this package. Different planners and modifiers (looking at you, heat method) will introduce different dependencies. While it all fits nicely into one library now, it will be a very large library by the time we are done.

It's like this mainly because of the sub-directory structure of the package. We've gone back and forth on this layout a few times, but to me the benefits of the sub-directories are to:

* separate the definition of the interfaces from their implementations (e.g. core vs. tool_path_modifiers)

* separate the implementations of different interfaces from one another (e.g. tool_path_planners vs. tool_path_modifiers)

* smaller self-contained CMakeLists

I wouldn't mind having a single library for this package. Is that possible with the sub-directory approach or would we need to change?

If we are not actually building these pieces of code as distinct libraries or modules any more, I really fail to see the benefit of leaving these all as separate sub-directories each with their own include and src folders. The current directory structure doesn't make a lot of sense to me, but I think it becomes unjustifiable complication when it no longer parallels the build structure.

[jdlangs]

Even with full implementations of everything, this won't be that big of a project by open-source standards, which routinely bundle dozens of classes into a single library.

Different planners and modifiers (looking at you, heat method) will introduce different dependencies.

A strange dependency would be a very valid reason for not including an implementation in the main library. It may not even belong in the package in that case.

It's like this mainly because of the sub-directory structure of the package...

I mostly agree with David I don't think there's much benefit in trying to separate these things in the directory hierarchy. I think the main thing is interfaces don't need to be considered something special that ought to be organized separately from implementations. Consider the organization of a project like rclcpp which is a huge library stuffed with interfaces.

I'm not saying I would prefer it to change from the subdirectories, just that in the end the file organization doesn't matter, users want to get their info from doxygen and reference docs instead of the github repo.

[marip8]

I'm on board with changing to a structure without sub-directories and building a single library. If we need to go back to sub-directories to support planners with additional dependencies we can decide how to handle it then

[jdlangs]

Thanks for considering my feedback. I think this is looking good and I'm looking forward to the next steps.

[drchrislewis]

@DavidMerzJr @marip8 @jdlangs This PR fails to fix significant duplications of code. For example nearly every planner forms a sequence of unevenly spaced surface points from which an evenly spaced sequence of poses needs to be generated. Each re-implements methods for evenly spaced sampling and for estimating the x,y&z axis of the poses. Some are better implementations than others, there are certainly bugs. Adding a formal layer at the top does nothing to reduce the duplication of code. One way to do it would be to have the planner's each output both a list of indices into the mesh or the points them selves indicating the unevenly spaced sequences. Then we could let the modifier chose how to evenly sample and to select the x,y,z axes.

[marip8]

This PR fails to fix significant duplications of code.

This PR contains only the interfaces for the rest of the porting activity. This set up will make the code much more modular than the current implementation, so the reduction in code duplication will come in future PRs when we actually migrate functionality. Bear with during this process and this will become evident

For example nearly every planner forms a sequence of unevenly spaced surface points from which an evenly spaced sequence of poses needs to be generated. Each re-implements methods for evenly spaced sampling and for estimating the x,y&z axis of the poses. Some are better implementations than others, there are certainly bugs. Adding a formal layer at the top does nothing to reduce the duplication of code.

Our plan is to formalize the various implementations of the existing smoothers, resamplers, etc. you are describing into independent modular modifiers that will be accessible to all planners rather than being reimplemented differently in each planner. This is in fact a reduction in code duplication because each type of smoother, resampler, etc. will only have a single implementation in the form of a tool path modifier.

The sort of mesh-relative waypoint re-sampling that I think you're talking about might be a slightly challenging case for our current modifier interface. I think there are some ways we can get around it, but we will consider those early in the porting process so we can evaluate whether the modifier interface requires more information (i.e. the mesh used to generate the tool path)