-
Notifications
You must be signed in to change notification settings - Fork 65
WIP: integration of unity_ros2 improvements and port to crystal #33
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
Conversation
- nested types - unit tests - rclcs overhaul to crystal version and structure changes Also added my own fixes to build and remaining code - extended interfaces to provide something useful - replaced obsolete rcl_ok() call - adapted examples to new library semantics - introduced ros2_dotnet_crystal.repo, which supports a smaller build with existing Crystal install
|
@adamdbrw thanks, I really appreciate the effort, but this is impossible to review. Can it be broken into smaller pull requests? Also, I'm not comfortable with including work from someone else's, it should be Samuel who submits the pull request, to ensure that all is ok, copyright-wise. |
|
@esteve
|
|
@esteve I believe these are not normal circumstances, since we are creatively integrating two bodies of work with plenty of differences. There are methods to handle this kind of case, i.e. a dedicated code-base review after we pass Unit Tests and CI. I also understand there's anxiety when your child mutates into something alien. I like small pull requests just like any other guy, and I seek no glory. It is of course a question whether the benefit is worth the effort for you and other ros2_dotnet users. |
|
FWIW my current thinking is to focus on getting a 'minimal' standalone build of dashing running in a UWP container, using the C++/WinRT native layer (no C# required). This ought to serve as a foundation for deploying ROS2 within UWP (including the various patches to My understanding is the the rcldotnet/rclcs stuff should ride on top of that rather independently, right? By the way @esteve is there any significant need to fully support crystal now that dashing is out? I'd rather skip straight to the latest. |
Except that one of the bodies of work started two years ago, whereas the other is a fork that could have been integrated as patches from the beginning. This pull request wasn't created in a vacuum completely separate from the rest of the world.
Why not just contribute the test suite? That's what I told you by email.
There's nothing of that, look at https://github.com/ros2-rust/ros2_rust for example. A user saw that ros2-rust already existed and instead of forking it and evolve it on their own without contributing back, he decided to submit a pull request (ros2-rust/ros2_rust#3) that included a lot of improvements, but in a way that was humanly reviewable. The outcome? I added that user to the team and he and I are now collaborating, he's actually taking the lead on the project. Moreover, what's the point of replacing one project with another? I just don't understand. If you like the other project, seriously, use it, I will not force anyone to use any of the projects I've written. I'm doing this in my spare time for the fun of it.
However, why should I make the effort of integrating a third person's work (Samuel)? Why couldn't that person submit this pull request? Your effort is laudable, but I just don't understand any of this process. Why isn't Samuel doing this work instead of you? |
|
@theseankelly that makes the most sense and it's much easier to iterate that way. Ensuring that ROS 2 Dashing as is (up to rcl) can be built for UWP would pave the way for any .NET implementation to support the HoloLens. I'll set up the CI to use |
|
@esteve |
Work in progress: this is not a master-ready material, but please create a working branch so we can coordinate work between 3 people easily and make other pull requests to it.
This is a natural step towards Dashing port, which should follow once we get this to a satisfying level.
Current status:
How to run:
Required work:
Commit message:
Integrated improvements from unity_ros2 project:
Also added my own fixes to build and remaining code