Allow configuring maximum target count#2338
Conversation
|
Moving from #2319 to allow contributors to edit. |
a4ab063 to
f8db6d5
Compare
mcm001
left a comment
There was a problem hiding this comment.
I don't see any tests showing that the upgrade logic works as expected?
b085972 to
af01e64
Compare
I'd like to get #2331 in, then I'll use that config to test. |
383c711 to
5c56037
Compare
864ca60 to
91aa4a3
Compare
photon-core/src/test/java/org/photonvision/common/configuration/SQLConfigTest.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/vision/pipe/impl/SortContoursPipe.java
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/vision/pipeline/AdvancedPipelineSettings.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/vision/pipeline/ObjectDetectionPipelineSettings.java
Show resolved
Hide resolved
photon-core/src/test/java/org/photonvision/common/configuration/SQLConfigTest.java
Show resolved
Hide resolved
c899cf8 to
553168a
Compare
…ancedPipelineSettings.java Co-authored-by: Gold856 <[email protected]>
553168a to
c470cca
Compare
mcm001
left a comment
There was a problem hiding this comment.
Also looks to be missing new unit tests showing that the limit is respected. Would accept test evidence instead
photon-core/src/main/java/org/photonvision/vision/pipe/impl/Draw2dTargetsPipe.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/vision/pipe/impl/SortContoursPipe.java
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/vision/pipe/impl/SortContoursPipe.java
Outdated
Show resolved
Hide resolved
…pipe/impl/SortContoursPipe.java
f026a65 to
b912fef
Compare
b912fef to
f6a2f2c
Compare
|
Right now:
So just confirming that the correct current upper limit is 127. Any change to our pack/unpack code and datatypes silently breaks older unpack versions. this is deeply terrifying to me mid season. I should have included a schemaVersion so we can handle this in the protocol, but I didn't and now we're stuck with this until 2027.
|
32b2d52 to
b22d54c
Compare
Gold856
left a comment
There was a problem hiding this comment.
There are a bunch of off-by-one errors.
So just confirming that the correct current upper limit is 127
Sort of incorrect. The max number of targets is 128. The max index, therefore, must be 127. There are several instances of List#size being (incorrectly) directly compared to Byte.MAX_VALUE. The list size should be subtracted by one first.
There's also a bunch of duplicated code checking the list size in various different pipelines. There must be a better place to do that...
photon-core/src/test/java/org/photonvision/common/configuration/SQLConfigTest.java
Show resolved
Hide resolved
photon-core/src/test/java/org/photonvision/vision/pipeline/AprilTagTest.java
Show resolved
Hide resolved
|
Concur wrt the duplicated code, but j do think that the pipeline is the correct place for the limit to live. However, there is not an off by one in my implementation. Java bytes max value is 127. 0 means 0 targets, 1 means one target, and 127 means 127 targets. 128 is not representable by a Java byte. Therefore the length of the target list must be on [0,127]. QED |

Description
We wanted to track more than 10 objects in an Object Detection pipeline, so we added a "Maximum Targets" setting for when "Show Multiple Targets" is enabled. The default value is 10, so no existing configuration should change, though a "Show multiple targets" setting is redundant in theory.
None of the documentation mentioned this 10-object limit, so nothing there changed for now.
I also did some homework on if we can at present send more than 127 targets. Across our decode clients
photonvision/photon-lib/py/photonlibpy/packet.py
Line 109 in 7745721
So just confirming that the correct current upper limit is 127. Any change to our pack/unpack code and datatypes silently breaks older unpack versions. this is deeply terrifying to me mid season. I should have included a schemaVersion so we can handle this in the protocol, but I didn't and now we're stuck with this until 2027.
Changes
Meta
Merge checklist: