mjansen4857 / pathplanner

A simple yet powerful path planning tool for FRC robots
https://pathplanner.dev
MIT License
398 stars 125 forks source link

Add ability to inject parameters into commands from the path planner GUI #347

Open WarrenReynolds opened 1 year ago

WarrenReynolds commented 1 year ago

I've had this idea on the wish list before and I release its hard to do because the commands are declared in code before they are added to the command map so there was no way to modify the member variables of the commands as the auto is being constructed at run time, how about this as an idea.

Anywhere there is command in the auto, have the ability to add a list of parameter names and values as well as a parameter type. Then use the preferences table on the rio as a place to put this information. The data types that are allowed to be used are the same as the types that are allowed to be saved in the parameters table (string, int, double, float, boolean, long)

The naming convention of the RIO parameter table key could be the command name as used in the command map, then and an underscore and then parameter name.

This would give the command the effective ability to pull values from the Path Planner GUI. In the running logic of the command, it would read the preferences table to get values that it needs to operate. Eg parameter could be called AutoAimShoot_DefaultDistance with a float value of 3.7

An example case. In the 2022 game we had an auto aim and shoot command that was used in our 5 ball auto. It would use the limelight to calculate an exact distance and heading correction to score in the high goal. Once the robot was aligned (not usually needed as pathplanner usually got us pretty dam close) it would use the calculated distance from the limelight to spin the shooter to exactly the right speed and then feed the balls into the shooter. This all worked perfectly if the limelight got a valid target, but if it didn't, the auto would just stop. So, we added a parameter to our "auto aim and shoot command" that was DefaultDistance. If the limelight didn't get a valid target, it would use this parameter value to spin up the shooter and shoot regardless. This was all before pathplanner had the ability to build the command group completely from the GUI, so when had these manual values hard coded in the auto sequential command group for each auto, There was a fair bit of pain in building these nested auto group as we needed to continually hand over the subsystem when auto aiming and then branch out with another command group that required the drive for the next segment of the path, etc, etc.

Now that pathplanner has evolved and can build the auto on the fly completely from information in the GUI, the only thing left would be the ability to add parameters to command. Other use case could be motion magic max vel, and max accelerations for arm or elevator movements, shooter pre spin up speeds, number of items to score, basically any value that a command needs that isn't going to know at compile time.

The RIO parameters table is a bit of global variable hack that probably isn't "correct" coding practice, but it does solve this problem of passing values to commands from the Path planner GUI.

jonahsnider commented 1 year ago

This seems like it goes outside of PathPlanner's domain, and introduces a ton of complexity to the GUI, library, and GUI + library consumers. Some autos are going to require you to write some robot code to implement them, and that's okay.

WarrenReynolds commented 1 year ago

This seems like it goes outside of PathPlanner's domain,

Sure, but path planners domain is ever changing. Two years ago, you could only follow a path, then you could follow a path and fire an command at a stop point, and then you could fire a command at a point anywhere along a path, then you could build every possible mix of sequential and parallel groups of command with waits before, after and during, with deadlines and races at any points on the path. So for me the domain is every changing, and this is a possible next evolution in path planners auto builder journey. It's so lovely to be able to build a full auto command group from entirely with in the GUI. It's is only an idea and I'm happy for Micheal to close it off if it not feasible

Some autos are going to require you to write some robot code to implement them, and that's okay.

Yep, and in the case above we could have still used the new Path Planner's all-in-one auto builder, we would have just needed multiply instances of the AutoAimShoot command in the command map (something like AutoAimShoot2100, AutoAimShoot3200, AutoAimShoot4600) where the four digit number was the default distance.

If this doesn't get any traction and anyone is interest in a work around, I think I have a way it could be done with zero changes to the GUI. and only very minor changes to the path planner lib files. You would need to pull the path planner lib files down locally so that you can modify two files (EventManager.cpp at line 18 and CommandUtil.cpp at line 27) If the command name pulled from the JSON file has an underscore (or some other special character you choose) in it, then everything before the underscore is the actual command name to search the command map for and everything after the underscore is the parameter value to use in the command.

Then to use this value at run time, your set the name member variable of the wrapped command using .SetName() to the the full value extracted from the JSON file. Then inside the init method of the command you could read the name member variable of itself and extract the parameter value (the number after the underscore) from the name text and use this in your command at your will. This way nothing needs to be done to the path planner GUI and as a bonus any command logging that uses the commands name member variable will actually be better because you will see not only the command name but also its parameter in any command scheduler logs.

This all falls in a heap if the proposed changes of checking if commands in the command map match between the PP GUI and robot source code is implemented as proposed in another issue. Also not ideal to pull the path planner code down locally as then you have to manually update every time an update is released.

Hey @mjansen4857 , surely you could put this tiny mod into your source. Make the special character something obscure like % or ^. A couple of small mods to the EventManager and CommandUtil files and its done. The name field of the wrapped command it not being used as it stands at the moment and what people do with the Name field is up to them.

All it would need is one line in the wiki, "When entering commands names in path planner GUI, if a command name includes {special symbol}, everything after {special symbol} will be ignored when searching for the command in the command map and the commands name will be set to the full name including {special symbol}.
Call it a feature that allows you to custom the name values of a command that is included from the Path Planner GUI.

mjansen4857 commented 1 year ago

There are ways to do this, and you mentioned a couple, but I think that they are fairly unintuitive/a bit hacky to me. The alternative here like you mentioned it to just add different variations to the event map. My team does this ourselves with commands like "intakeSlow", "intakeFast", etc. When making autos, you rarely need that infinite adjustment of parameters because you're pretty much going to be using just a handful of different intake speeds, elevator heights, shooter speeds, etc. So, the few seconds it takes to add a couple more variations of a given command to the event map is much more efficient than having the user implement code to parse a command name, pass parameters around, etc. Not to mention, you could adjust the command in the event map, which will now be applied to everywhere it is used, instead of having to go through all of your autos in the GUI and change the parameter there.

In order for me to want to implement something like this, it would need to be almost no additional effort from the user perspective than it currently is to add commands to the event map. I have a couple ideas for that, but it is likely not a 2024 thing at this point.

WarrenReynolds commented 1 year ago

Fair enough, I just really like the ability to adjust things from PP GUI, not modifying the source, recompile, etc. I'm bit scared from a couple of years back when we had some really messy c++ code spread of many individual files (every command in it's own file, nothing in-lined), very slow school owned coding laptop that we couldn't disable antivirus on and our build times where 15 minutes plus. So we could only try 4 different intake speeds per hour if we wanted to make a change. We have since moved to using the preferences table for anything that needs adjustments, so its not as bad now, hence the original idea of updating the preferences table from inside PP.

In order for me to want to implement something like this, it would need to be almost no additional effort from the user perspective.

I thought I ticked that box, all the end user would be doing is changing the command name in the GUI from AutoAimShoot to AutoAimShoot%3200. They could still use AutoAimShoot if they wanted to, but if they used the second version, the same command would be added to the Auto Command group, just with a different internal name. Like I said it could solely be used as a logging tool aid.

Anyway, it doesn't look like this is going to fly, so we will just have to come up with something ourselves.

Cheers and thanks again for making this fabulous product free to all FRC teams, happy for you to close off this issue if you want.

mjansen4857 commented 1 year ago

I thought I ticked that box, all the end user would be doing is changing the command name in the GUI from AutoAimShoot to AutoAimShoot%3200. They could still use AutoAimShoot if they wanted to, but if they used the second version, the same command would be added to the Auto Command group, just with a different internal name.

Yes, but then you would need to do some sort of parsing of the name in the command in order for it to change anything.

I'll leave this open, I've had this general idea for a while. I just need to find a way to do it I'd be happy with. So it will probably just stay open a while cuz I doubt that will be this year. I need to focus on finishing everything up instead of adding more big changes for now.

WarrenReynolds commented 1 year ago

Sounds like a case for Advanced Polymorphism. We are trying to make the runtime pointer to mammal which is actually a pointer to cat, purr. (If you have read, SAMS Teach yourself c++ in 24 hours you will know this reference).

I think you will need to create a PPCommand Class that is derived from the FRC Command Class. This PPCommand class has some sort of setter function that allows you set <name, value> pairs, a bit like the command map. These name values pairs are stored in a vectored array as a member variable of the PPCommand class. Because the PPCommand class is inherited from the inbuilt FRC Command class any command that is of type PPCommand should work fine in regards to chaining commands together, grouping, etc.

The command map could then have a mixture of normal FRC commands and/or PPCommands because it relates a name to a pointer to command (standard polymorphism).

When path planner is building the Auto group at run time, and it comes across a command to add to the group from the JSON file that has some parameter pair values associated with it. (Obviously the PP GUI will need a modification to add optional named parameter pairs to commands). The auto builder should attempt a dynamic_cast to < PPCommand > on the pointer it has to command in the command map. If this cast succeeds (ie doesn't return null) then the auto builder can call the setter function that is defined in the PPCommand class definition to set a name/value pair inside the instance of the PPCommand. If the dynamic cast fails with returning a null pointer, than this command is not of type PPCommand and it can't take parameters, so the parameters are just ignored and the command is added just like it has been in the past.

Then if people want to use this information that is injected from the Auto builder, they have to include that logic in the _init() function of their command. This is the only way I can see this being done without the need for some shared global table (like RIO preferences)

Then you add functionality to the PP GUI that pre-populates these optional parameters with coordinates, or distances to a point, etc. Then if a point is moved in the GUI the parameters would auto update. The sky is the limit here.

WarrenReynolds commented 6 months ago

@mjansen4857 Hey Micheal, is this idea still on the todo list for the 2025 release. It would have been great for us this year with shooting from many different spots in our autos.