WIP: Temporary PR to view code (do not merge) #41
No reviewers
Labels
No labels
Autos
Climber
Indexer
Intake
Lighting
Shooter
Vision
bug
documentation
duplicate
feat
good first issue
help wanted
invalid
question
revert
wontfix
bug
documentation
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
ironriders/rebuilt!41
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Vision-V2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Just creating this to give feedback to Elliot.
making this a draft to enforce not merging
You do a lot of work in VisionCamera. The point of the Subsystem paradigm is to primarily do work in subsystem files. I would refactor this as such.
@ -0,0 +1,183 @@package org.ironriders.vision;I'd remove HTML tags.
@ -0,0 +1,183 @@package org.ironriders.vision;This all needs to be deleted - Elliot
You don't do any filtering of good vs bad tags. We determined that this was necessary to not get bad results.
@ -0,0 +1,183 @@package org.ironriders.vision;Better names:
MULTI_TAG_SIMULATED
MULTI_TAG_ACTUAL
@ -0,0 +1,183 @@package org.ironriders.vision;OLD should be something like LEGACY_VISION_CODE
@ -0,0 +1,183 @@package org.ironriders.vision;estimator -> poseEstimator
@ -0,0 +1,183 @@package org.ironriders.vision;Write a helper method:
isSimulation
This code is just very unreadable and overcomplicated in general. Sorry if im being to harsh TwT.
@ -1,316 +1,165 @@package org.ironriders.vision;PhotonCameras are not thread safe, you can not parallelStream over them. Use normal stream() instead.
@ -0,0 +1,183 @@package org.ironriders.vision;also i think OLD is just unused
@ -0,0 +1,183 @@package org.ironriders.vision;Actually you can have the enum have this value
@ -0,0 +1,183 @@package org.ironriders.vision;Add (isSimulation)
@ -0,0 +1,183 @@package org.ironriders.vision;We can omit the
true : falsehereYou also do a lot of recursion, which is not optimal. Also, you often assume what you are indexing over is not empty or null, which is not necessarily the case.
see https://en.wikipedia.org/wiki/The_Power_of_10:_Rules_for_Developing_Safety-Critical_Code
@ -0,0 +1,183 @@package org.ironriders.vision;Could also be
IllegalStateException@ -0,0 +1,183 @@package org.ironriders.vision;Adding more of the variables and their values will help if this exception is thrown
@ -0,0 +1,183 @@package org.ironriders.vision;you should also probably try to go down a failure path that recovers instead of just exiting. e.g. just throw a warning and continue without vision
@ -0,0 +1,183 @@package org.ironriders.vision;you can even have isSimulation() be a method on the enum here.
@ -0,0 +1,183 @@package org.ironriders.vision;Add "throwaway values"
@ -0,0 +1,183 @@package org.ironriders.vision;better name is
getSingleTagEstimations@ -0,0 +1,183 @@package org.ironriders.vision;Better name is
getMultiTagEstimations@ -0,0 +1,183 @@package org.ironriders.vision;rename to
target@ -0,0 +1,183 @@package org.ironriders.vision;Can we either (or both):
@ -0,0 +1,183 @@package org.ironriders.vision;do we know what units this distance is in? We should rename the variable.
@ -0,0 +1,183 @@package org.ironriders.vision;30 is another magic number here.
@ -0,0 +30,4 @@** The main logic for calculating standard deviations is in this class.*/public class VisionCamera {This entire class needs its methods evaluated for changing to
private@ -0,0 +1,183 @@package org.ironriders.vision;Does this method even need to exist or can you just access
stdDevsin the methods that need it? Are other classes accessing it?@ -0,0 +1,183 @@package org.ironriders.vision;thisseems unnecessary here.@ -1,110 +1,105 @@package org.ironriders.vision;it's ok to not have these comments on each one, but it should be on the first one.
@ -1,110 +1,105 @@package org.ironriders.vision;The code here needs to be restructured to be more readable and understandable.
@ -1,110 +1,105 @@package org.ironriders.vision;Consider constants or an enum instead of these raw strings.
@ -1,110 +1,105 @@package org.ironriders.vision;If you're not the person getting the actual values, it would be helpful to have more detailed instructions on the source.
@ -1,316 +1,165 @@package org.ironriders.vision;"We use this class instead of"
@ -1,316 +1,165 @@package org.ironriders.vision;Better name here.
VisionLogEntry?@ -1,316 +1,165 @@package org.ironriders.vision;Quesetions:
@ -1,316 +1,165 @@package org.ironriders.vision;Another issue with the names here is that it doesn't explain if "Single / Multi" is referencing the number of cameras or the number of tags.
@ -1,316 +1,165 @@package org.ironriders.vision;poseEstimateConsumer@ -0,0 +1,183 @@package org.ironriders.vision;Changed name to getPoseSingleTag, if that works
The 'throwaway values' comment refers to adding values (such as distance, ambiguity, etc...) for which we should throwaway tag estimates, should, after testing, it prove necessary. Previous code was already tuned for this, maybe look there for values if they prove to be needed.
@ -1,316 +1,165 @@package org.ironriders.vision;Thanks!
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.