search

SleipnirGroup / Choreo

A graphical tool for planning time-optimized trajectories for autonomous mobile robots in the FIRST Robotics Competition.
BSD 3-Clause "New" or "Revised" License
150 stars 46 forks source link

Replace no-args trigger factories(done(), active(), running(), etc.) with public properties #904

Closed Daniel1464 closed 2 weeks ago

Daniel1464 commented 3 weeks ago

Resolves https://github.com/SleipnirGroup/Choreo/issues/897

Since these triggers are used so often, they can help significantly reduce clutter in the code. Before:

path1.done().onTrue(...)
path2.active().onTrue(...)

Now:

path1.done.onTrue(...)
path2.active.onTrue(...)

This also reduces the number of allocations.

Daniel1464 commented 3 weeks ago

Btw am working on c++ implementation soon; just kinda busy this week

Daniel1464 commented 3 weeks ago

/format

oh-yes-0-fps commented 2 weeks ago

This should not be done, if you want to reduce allocations keep them stored as members internally.

Having access be a method is more flexible allowing for extra arguments, there really isn't a benefit to doing it like this besides removing the parent which isn't worth it for giving up coherency with the rest of the API.

spacey-sooty commented 2 weeks ago

This restricts us in terms of future API changes so we won't do it but if you'd like to make these functions singletons that would be accepted

Daniel1464 commented 2 weeks ago

Having access be a method is more flexible allowing for extra arguments, there really isn't a benefit to doing it like this besides removing the parent which isn't worth it for giving up coherency with the rest of the API.

I dont think this stops us from adding a done()/active() method that takes in arguments.

Having no-args methods be replaced by properties is also pretty consistent as well. In general, I think that if we don't need something to be a method it should not be a method, and it should be properly represented as what it is.

spacey-sooty commented 2 weeks ago

Having access be a method is more flexible allowing for extra arguments, there really isn't a benefit to doing it like this besides removing the parent which isn't worth it for giving up coherency with the rest of the API.

I dont think this stops us from adding a done()/active() method that takes in arguments.

Having no-args methods be replaced by properties is also pretty consistent as well. In general, I think that if we don't need something to be a method it should not be a method, and it should be properly represented as what it is.

The issue with this is it makes it very hard to change the implementation of these Triggers without breaking the API. With a method this is reduced.

Daniel1464 commented 2 weeks ago

The issue with this is it makes it very hard to change the implementation of these Triggers without breaking the API. With a method this is reduced.

ok honestly u guys prob have more foresight than me on this, so i'll trust your intuition.