FlexBE / flexbe_app

The classic user interface (editor + runtime control) for the FlexBE behavior engine. See the flexbe_webui for latest
BSD 3-Clause "New" or "Revised" License
48 stars 49 forks source link

FlexBE app seems to load only the first EventState per module #45

Closed cheffe112 closed 4 years ago

cheffe112 commented 4 years ago

First of all, thanks @pschillinger for developing FlexBE and all the work you put into it; in fact, this is a great tool with lots of useful features!

I came across some unexpected behavior (that is, unexpected to me) when transforming my existing smach states to FlexBE states. I've grouped several smach states inside one module, and when I've made FlexBE states out of them, it seems that FlexBE loads only the first state per module and ignores all others in the same file.

I guess this is somewhat related to https://github.com/FlexBE/flexbe_app/issues/42 and might also come from https://github.com/FlexBE/flexbe_app/blob/master/src/ros/ros.js, however I was unable to find where exactly this could be avoided. Anyhow, this may as well be desired behavior; if so, I guess it would be helpful to add a remark in http://wiki.ros.org/flexbe/Tutorials/Developing%20Basic%20States - or maybe it is already somewhere in the documentation and I've skipped it accidently.

pschillinger commented 4 years ago

Thank you for the feedback!

This is indeed the way it was intended to be implemented and I agree that it should be added prominently to the basic states tutorial.

To give a short motivation for this decision (and happy to hear your thoughts on it), the main idea was to keep states as organized and re-usable as possible. For example, the one-state-per-file paradigm enables:

That being sad, I don't see any serious drawbacks. It just requires a bit more refactoring in cases like yours where an existing system is switched, but to be fair, I also see such a refactoring as worthy towards the level of re-usability that a growing system should have.

Looking forward to your thoughts on this.

cheffe112 commented 4 years ago

Sounds great; very reasonable rationale you're giving there and makes sense to me. I'm transforming all my states right now, but the amount of refactoring necessary is indeed minimal.

So I agree that this should remain the way to follow; maybe you could add this remark in the basic states tutorial? I'd do it myself, but you know the documentation much better than me so you'd know immediately where to put it best, I guess.