search

rosin-project / rxros

Reactive programming for ROS
BSD 3-Clause "New" or "Revised" License
48 stars 6 forks source link

Factor out rxros_tf from rxros #8

Closed wasowski closed 4 years ago

wasowski commented 5 years ago

See issue #3 . It should be possible to get the core part of rxros be completely independent from tf.

gavanderhoorn commented 5 years ago

Some initial work: https://github.com/rosin-project/rxros/compare/master...gavanderhoorn:refactor_tf_support.

gavanderhoorn commented 5 years ago

@henrik7264: I've noticed that observable is currently a class (here), but all methods are static (here fi).

Would it be possible to make an observable namespace instead of a class? That would make adding to the rxros namespace easier (from rxros_tf fi).

For now I've created an additional rxros_tf namespace to allow the second observable class to co-exist with the one in rxros.

(note: I've not actually used this yet, so it could be that this is a silly/stupid question/suggestion)

gavanderhoorn commented 5 years ago

And just an observation: besides this:

https://github.com/rosin-project/rxros/blob/5bb5fb3fab9ab06e846728a44196c57434312b2a/rxros/include/rxros/rxros.h#L229

rxros_tf appears to be independent from rxros itself (the TF related code only uses rxcpp, roscpp and tf).

henrik7264 commented 5 years ago

Hi Gijs

I shall look at it later this evening.

Regards Henrik

henrik7264 commented 5 years ago

I actually think there is a good chance that the class may be replaced by a namespace. Thinking about it I actually better like this approach.

I shall check it later to night. I am on work now.

henrik7264 commented 5 years ago

Here is a proposal for how the rxros.h and rxros_tf.h could be made. rxros.zip

gavanderhoorn commented 4 years ago

@henrik7264: looking at the contents of the zip file it appears the biggest changes are the introduction of the rxros_tf.h header and the fact that observable is now a namespace. Correct?

That all looks OK to me.

Would you want to submit that as a pull request against this repository?

henrik7264 commented 4 years ago

@gavanderhoorn yes the zip file is a proposed split of the rxros.h into a rxros_tf.h and a rxros.h without tf. The second change is to use a namespace instead of the observable class. Yes it would be nice to have this as a pull request against this repository.

henrik7264 commented 4 years ago

@gavanderhoorn could I kindly ask you to make the necessary preparation with respect distributing the files to the appropriate repositories and setup up all the nice ROS workspace magic. I shall be happy to review and merge them later.

gavanderhoorn commented 4 years ago

@henrik7264: see #24, which is a continuation of the work I mentioned in https://github.com/rosin-project/rxros/issues/8#issuecomment-526909032 combined with the changes you proposed in your .zip in https://github.com/rosin-project/rxros/issues/8#issuecomment-526928704.

If you could review that would be great.