Skip to content

PhotonPoseEstimator#update is a leaky abstraction; use strategy methods instead #1835

Open
0 of 1 issue completed
@Gold856

Description

@Gold856

Is your feature request related to a problem? Please describe.
The update method in PhotonPoseEstimator attempts to abstract away the different requirements of different strategies by using overloads with optional parameters that are only used by certain strategies. This is a very leaky abstraction and can cause mistakes in user code, such as forgetting to provide calibration parameters to a strategy that needs it. The addition of strategies like constrained PnP have made the abstraction more leaky, since it's impossible to set it as a fallback strategy due to needing a seed pose from either multitag results or the fallback strategy, causing #1805.

Describe the solution you'd like
Instead of having one update method and separate enums to control the strategy, expose the strategy methods we already have to the user. This gives the user maximum control over how their data is processed. If a user only wants pose estimates from multitag results, they can choose to only call the multitag strategy method. If a user wants to use multitag results, then constrained PnP, then lowest ambiguity, they can do that in a simple manner:

var multiTagResult = photonEst.multiTagOnCoprocStrategy(result);
if (multiTagResult.isPresent()) {
    return multiTagResult;
}
var seedPose = photonEst.lowestAmbiguityStrategy(result).get();
if (seedPose.isPresent()) {
    var cpnpParams = new ConstrainedSolvepnpParams(false, 1, seedPose.estimatedPose)
    var cpnpResult = photonEst.constrainedPnpStrategy(result, camMat, distCoeffs, cpnpParams);
    if (cpnpResult.isPresent()) {
        return cpnpResult;
    }
}
return seedPose;

Describe alternatives you've considered
A stricter type system to prevent people from using the wrong strategies as fallback strategies. This helps with user error, but does not solve the fundamental issue of update being a poor abstraction.
Reorganizing library internals to always make empty Optional possible. This helps with crashes due to strategies being used incorrectly, but still has edge cases (like using multitag and constrained PnP together.)

Additional context
Full discussion: https://discord.com/channels/725836368059826228/725846784131203222/1352482575331098707
One thing to be aware of is documentation. Since the user needs to handle all of the Optionals, we need to have robust documentation and examples for people to copy.

Additionally, we should also be careful about where we place Optional checking, both in docs/examples and in library code. I would say there's never too much, but it is easy to forget to check if data actually exists before calling another method that needs it.

Sub-issues

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions