-
Notifications
You must be signed in to change notification settings - Fork 57
initial docs for sim overhaul [WIP] #283
initial docs for sim overhaul [WIP] #283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source/docs/programming/photonlib/images/ExampleGeneratedFrame.png
Outdated
Show resolved
Hide resolved
Co-authored-by: Mohammad Durrani <[email protected]>
Co-authored-by: Mohammad Durrani <[email protected]>
Co-authored-by: Mohammad Durrani <[email protected]>
Just a quick comment the simulation-deprecated was the original documentation that is currently live on the website with the addition of (deprecated) to the title. I am totally down to edit it but don't know if that is fully in scope for this PR. Just let me know |
…nvision-docs into jheidegger-sim-overhaul
Forgot that some of that stuff was old docs. Don't bother updating it then besides putting deprecated in parenthesis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amquake pls review
Sorry for the late response. I started reviewing but squirrel-brained and rewrote the simulation page, which I PRed here: jheidegger#1. This links to the updated examples in PhotonVision/photonvision#913, which would mean this pr waits on that. I preferred putting in descriptive code blocks instead of RLIs, hopefully that isn't too much of a maintainability concern. @jheidegger @mdurrani808 Lmk your thoughts |
We would prefer RLIs when possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got through almost everything. Looks real darn solid. Comments are optional/suggestions, feel free to close without resolution.
|
||
You can use this to help validate your robot code's behavior in simulation without special wrappers or additional hardware. | ||
Simulation is a powerful tool teams can use to validate their robot code without needing access to a physical robot. Read more about `simulation in WPILib <https://docs.wpilib.org/en/stable/docs/software/wpilib-tools/robot-simulation/introduction.html>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reword sugestion:
Simulation is a powerful tool for validating robot code without access to a physical robot
Fewer words, same meaning.
|
||
.. warning:: Not all network tables objects are updated in simulation. The interaction through PhotonLib remains the same. Actual camera images are also not simulated. | ||
With this approach required information about the camera and target properties is straightforward, and users can easily setup and experiment with PhotonLib in simulation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I worry about statements like "X is straightforward" - it can be discouraging if a student is stuck on a problem, sees the line in docs, and then wonders "if it's straightforward, why can't I figure it out?"
I'm assuming the intent of the line is to explain and justify the choice for what is in/out of scope for simulation?
Assuming so.... potential reword:
"This scope was chosen to balance fidelity of the simulation with the ease of setup, in a way that would best-benefit most teams"
|
||
A prerequisite for simulating vision frames is knowing where the camera is on the field-- to utilize PhotonVision simulation, you'll need to supply the robot pose periodically. This requires drivetrain simulation for your robot project if you want to generate camera frames as your robot moves around the field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested addition:
Need to supply the simulated robot pose periodically.
|
||
It requires a number of pieces of configuration to accurately simulate your physical setup. Match them to your configuration in PhotonVision, and to your robot's physical dimensions. | ||
A ``VisionSystemSim`` represents the simulated world for the camera, and contains the vision targets it can see. It is constructed with a unique label: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the label be globally unique? Or should it match the pipeline name that's in the real camera?
Addressed PR comments
Converting some internal documentation about setting up the new simulation environment
Would welcome any and all suggestions and improvements!
still todo