sertain / sertain-legacy

:robot: Write more for your robot with less
https://sertain.github.io/javadocs
MIT License
9 stars 3 forks source link

Add pathfinding functionality #1

Closed snowsignal closed 6 years ago

snowsignal commented 6 years ago

This doesn't execute commands to move the robot yet, because I want to build that off of our own command system, but the class at least exists now.

snowsignal commented 6 years ago

Made requested changes.

andrewda commented 6 years ago

Okay I'll merge once it starts to do stuff.

andrewda commented 6 years ago

Pretty minor because I can fix it manually, but please use imperative tense in the future. Add blah instead of Added blah.

snowsignal commented 6 years ago

Added things that do stuff, like you requested.

andrewda commented 6 years ago

Also I was wrong, please add the pathfinder jar and fileTree stuff. You're going to need Pathfinder-JNI and Pathfinder-Java. See: https://github.com/Open-RIO/GradleRIO/commit/86eda2a4afc0eaf27626adbba511a51260fbb26b#diff-1373a7db9df0e916c1cf63b9aec8f0e4R113

@SUPERCILEX: How is Jaci hosting Pathfinder?

andrewda commented 6 years ago

Yea, taking a step back and looking at this, it seems to just complicate things, not make anything simpler, which is the point of the Sertain Framework.

snowsignal commented 6 years ago

I don't really see what it is complicating here. @SUPERCILEX, could you elaborate on what part of the code has the potential to be bug-ridden, as you mentioned?

Also, @SUPERCILEX , I don't quite understand what you mean by 'hard-coding the units', since virtually all elements that go into the trajectory generation can be modified via the configureTrajectory function. The ones that are hard-coded are because they remain constant regardless of the robot path we are generating or the field we are on.

My biggest question is, if this is the wrong approach to aliasing the PathFinder library, what is the better way?

andrewda commented 6 years ago

I still don't see why we need to alias Pathfinder?

And right now generateEncoderFollowers() isn't returning anything so how are we going to get our encoder followers?

snowsignal commented 6 years ago

You can access the EncoderFollowers using PathController.left and PathController.right, defined at the end of the generateEncoderFollowers() function.

I thought we were going to alias PathFinder because we wanted a path generator for this library?

andrewda commented 6 years ago

I guess I don't really see the use of running generateEncoderFollowers() if it's just going to initialize stuff? That could probably be built in more smoothly.

If we do want a path generator in Sertain, then we should probably consider doing it ourselves. It's not that much code (see: meanlib) and it would be well worth it to have a better understanding of how path finding works. I just don't see much of a use aliasing Pathfinder here, unless it's made significantly easier and cleaner, which is difficult to do since it's already a pretty well written library.

snowsignal commented 6 years ago

The reason for creating generateEncoderFollowers is so that other configuration functions like addPath and configureTrajectory can be run first to set up the variables.

I mean, we could make our own path generator, but don't we want to avoid unnecessary complexity?

SUPERCILEX commented 6 years ago

I'll explain more at robotics, but:

I'll show you what we can do with Kotlin's extension functions and a few other niceties from the language.

snowsignal commented 6 years ago

OK, thanks for the clarification.

andrewda commented 6 years ago

Here's my ideal Path library:

snowsignal commented 6 years ago

OK, cool! I'll rewrite the code when I can.

andrewda commented 6 years ago

We should discuss at robotics and see how we're actually going to be using the library first. Might also be nice to integrate it with some deeper autonomous tools, like an AutonConfig that I discussed in the Google Doc.

snowsignal commented 6 years ago

Are we good to go, @andrewda and @SUPERCILEX?

snowsignal commented 6 years ago

Made requested changes.

SUPERCILEX commented 6 years ago

Will take a look

andrewda commented 6 years ago

Let's test this out on robot before merging.

snowsignal commented 6 years ago

Good idea.

andrewda commented 6 years ago

Compile latest Pathfinder from sources (https://github.com/JacisNonsense/Pathfinder)

SUPERCILEX commented 6 years ago

Thanks!