search

Levi-Armstrong / rosxslt

0 stars 0 forks source link

Review for ROS-Industrial #1

Open Levi-Armstrong opened 9 years ago

Levi-Armstrong commented 9 years ago

@gavanderhoorn, I was told you are probably the best person to have review this package and determine if it would be value added to ROS. I created a ROS wrapper for the XSLT language which has capability that are not available in the current xacro. Let me know your thoughts.

gavanderhoorn commented 9 years ago

I like the idea, this was probably inspired by the discussion in the 'abb naming' issue over at ros-industrial/abb?

Obvious first question: why introduce something new (xslt), and what was the rationale for not extending / fixing xacro?

Levi-Armstrong commented 9 years ago

Yea it was inspired by the discussion in the 'abb naming' and I was familiar with xslt and xpath but was not sure if it would work with ROS. That is what prompted me to put together a ros package to verify that it would work within ROS.

Obvious first question: why introduce something new (xslt), and what was the rationale for not extending / fixing xacro?

After researching the xslt and xpath more, it appeared to have all of the capabilities and more as a macro language for ROS. If xacro were to be extend / fixed it would basically be recreating what has been done by xslt and xpath. There maybe capability that xacro has that xslt and xpath dose not provide but from what I can tell from the xacro wiki it has everything and more. It may be possible to integrate the two where both can be interchangeable for the near future if that is more desirable. I am just not sure if there is any value in recreating something that is already available.

gavanderhoorn commented 9 years ago

I am just not sure if there is any value in recreating something that is already available.

I guess one of the raisons d'être for xacro is the fact that it is way easier to grok than xslt / xpath. As one of the goals of ROS is (or was?) to be user friendly, that could've been an important factor in the decision to create something new at the time. Also: implementing a full xslt / xpath processor is not a trivial task, and finding one that was bug-free enough could've been hard at the time.

Perhaps you could see whether someone at OSRF can dig-up some of the design decisions and / or requirements that guided the creation of xacro? The xacro/reviews page isn't too enlightening. I think it'd be a good idea to avoid whatever issues / pitfalls the original team ran into.

It may be possible to integrate the two where both can be interchangeable for the near future if that is more desirable.

XSLT is infinitely more powerful than xacro currently is, and personally I don't have any problems with the complexity of it. I definitely believe that any new system would be more much more valuable if it also supported the current xacro 'language'. Power-users could then use xslt templates, while casual users could stick to the more 'friendly' xacro syntax. This would also allow the tool to be a drop-in replacement for the xacro command, and maintain compatibility with already existing xacro files.

Levi-Armstrong commented 9 years ago

I appreciate you taking the time to review this package.

I understand that there most likely were circumstance that drove the development to this solution. I realized I should have been more clear in my comment so I want to clarify for those that may read this. I was not inferring that there was no value for xacro, just that there may not be value adding advanced features now when they are already available in other languages.

I will see if I can track down the design decision for xacro so I may verify that xslt is a suitable macro language for ROS.

I am working on a solution that will allow for xacro to be using in conjunction with xslt in the rosxslt package. The only solution that i have found thus far is during the processing of the includes, if it is a xacro to convert it to xslt. This will also provide a feature for developers to use this to convert there xacro to xslt providing dual functions.

gavanderhoorn commented 9 years ago

I was not inferring that there was no value for xacro [..]

@Levi-Armstrong: I weren't implying you were doing that :). As I wrote, I definitely see value in this. I just think it would be wise to avoid introducing new systems unnecessarily.

I will see if I can track down the design decision for xacro so I may verify that xslt is a suitable macro language for ROS.

Don't spend too much time on this, it was more of a "perhaps it would be nice if you .." suggestions. But I do feel it might provide you/us with some insights that could be helpful.

gavanderhoorn commented 9 years ago

@Levi-Armstrong: you might be interested in some of the development that xacro has recently seen, especially in jade-devel. See some of the recent closed PRs.

Levi-Armstrong commented 9 years ago

@gavanderhoorn: They appear to be moving toward a language with similar capability as xslt. Do you think there is a need for two macro languages?

gavanderhoorn commented 9 years ago

Who are 'they' exactly? And you mean the PRs seem to get xacro closer to the goals you had?

I still believe xslt is way more powerful than xacro will ever be. Whether that is something we want for ROS I don't know. I'd like it, but perhaps I'm not the best person to ask. In any case, if we add something that is compatible with regular xacro (or at least the products of that tool), I think it would be valuable.

Levi-Armstrong commented 9 years ago

Who are 'they' exactly? And you mean the PRs seem to get xacro closer to the goals you had?

The developer working on the jade-devel that you previously mention in the April 15 comment.

The current implementation has the same capabilities as those outlined in the xacro wiki except number 8 which I have not explored yet. I also think it would be valuable so I will keep working in all of the capabilities of xacro. Once all of the capabilites of xacro have been implemented what would be the next step in your opinion?

gavanderhoorn commented 9 years ago

"number 8" (converting xacros to urdf using a CMakeLists.txt) is not necessarily a 'xacro capability'; as long as your tool is callable from the command line taking the same arguments I'd say you got that covered.

Once all of the capabilites of xacro have been implemented what would be the next step in your opinion?

I'd say that depends on your goals: do you intend to replace xacro usage in the whole of ROS, or just for ROS-Industrial? If you feel you have a sufficient level of functionality, you could announce your tool on ros-users, see what kind of response you get. Perhaps a smaller 'beta-test' on the ros-i mailing list would also be a good first step.

If the tool is good enough and addresses many of the issues people currently have with xacro, I'm pretty sure it will get used (provided you got the backward compatibility sorted). The main benefit I see with something new is that you're not bogged down by all the legacy stuff you have to support.

gavanderhoorn commented 9 years ago

xacro docs on the ROS wiki were updated recently, documenting some of the recent improvements. See wiki.ros.org/xacro.

I think the support for arbitrary Python expressions in xacros brings it quite close to what your primary motivation for rosxslt was, am I right?

Levi-Armstrong commented 9 years ago

You are correct, it looks like they have implemented everything that was pushing toward the xlst macro language. The only other thing that is missing is the switch implementation but it can currently be implemented using if statements just not as clean.

gavanderhoorn commented 9 years ago

One advantage of rosxslt would be that we'd be able to use it on earlier ROS versions as well. The xacro improvements are locked to Indigo and Jade. Especially the more interesting ones.