Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Rework Camera Properties #49

Open
jangernert opened this issue Jan 11, 2018 · 2 comments · May be fixed by #61
Open

[Proposal] Rework Camera Properties #49

jangernert opened this issue Jan 11, 2018 · 2 comments · May be fixed by #61
Assignees

Comments

@jangernert
Copy link
Collaborator

Working on the RealSense2 implementation in MetriCam2 and using the MetriCam2.Controls I noticed a few things that in my opinion could be improved.

  1. Properties and their descriptors are 2 separate things, which are only linked via naming conventions. This leaves room for mistakes and makes things more cluttered.
  2. Allowed values of ListParameters are stored as a list of strings instead of their actual type
  3. Deep inside MetriCam2 conversions back and forth from actual types to strings are happening very frequently.
  4. This also means every type other than the few standard ones need special handling inside MetriCam2 AND the controls (e.g. Point2i(1920,1080) for resolution to a string representation like 1920x1080).
  5. This in turn means the code in MetriCam2 is very error prone (what if someone want's to have a ListParameter<Point2i> as well but defines its allowed values as strings looking like (32;27) instead?)

So the basic suggestion I want to make is:

  1. combine property and descriptor to one entity. This way the value and all its metadata are available at the same time.
  2. Have the allowed values of ListProperty available as its native type. So a value can easily be checked for validity without crazy conversions.
  3. Let every Property have a ToString function that can convert the value for usage in the UI. Of course with a basic overwriteable default implementation that should fit most use cases. More complex situations like the above mentioned resolution can simply overwrite it.

I hope so far everyone agrees with me.
Now to a rough prototype how I imagine things could be done. First off how you would currently define a RangeProperty:

public int GainColor
{
    get
    {
        CheckOptionSupported(RealSense2API.Option.GAIN, GainColorDesc.Name, RealSense2API.SensorName.COLOR);
        return (int)RealSense2API.GetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.GAIN);
    }

    set
    {
        CheckOptionSupported(RealSense2API.Option.GAIN, GainColorDesc.Name, RealSense2API.SensorName.COLOR);
        CheckRangeValid<int>(GainColorDesc, value, 0);
        RealSense2API.SetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.GAIN, (float)value);
    }
}

RangeParamDesc<int> GainColorDesc
{
    get
    {
        RangeParamDesc<int> res;

        if(this.IsConnected)
        {
            RealSense2API.QueryOptionInfo(
            _pipeline,
            RealSense2API.SensorName.COLOR,
            RealSense2API.Option.GAIN,
            out float min,
            out float max,
            out float step,
            out float def,
            out string desc);

            res = new RangeParamDesc<int>((int)min, (int)max);
        }
        else
        {
            res = new RangeParamDesc<int>(0, 0);
        }
        
        res.Description = "Color image gain";
        res.ReadableWhen = ParamDesc.ConnectionStates.Connected;
        res.WritableWhen = ParamDesc.ConnectionStates.Connected;
        return res;
    }
}

Compared to how I think it could work:

public RangeProperty<int> Gain = new RangeProperty<int>(-64, 64, 0)
{
    Name = "Gain (Color Sensor)",
    Description = "Gain of the Color Sensor",
    ReadableWhen = ConnectionStates.Connected,
    WritableWhen = ConnectionStates.Connected,
    CalcRangeFunc = () =>
    {
        // if(!IsConnected)
        // ... some fallback if camera is not connected
        Point2i gainRange = DemoCameraAPI.RetrieveGainRange();
        return new Range<int>(gainRange.X, gainRange.Y);
    },
    SetFunc = (gain) =>
    {
        // if(!IsConnected)
        // ... some fallback if camera is not connected
        DemoCameraAPI.SetGain(gain);
        return true;
    },
    GetFunc = () =>
    {
        // if(!IsConnected)
        // ... some fallback if camera is not connected
        return DemoCameraAPI.GetGain();
    }
};

Here is the complete code of the little prototype I wrote:
Gist

@stephenmartindale mentioned we maybe could use attributes to achieve the functionality we want. Although I knew attributes existed, I never realized you could define your own. So I just had a rough look at some of the docs, but haven't figured out all the details of how we would implement everything with attributes yet.

@f00f
Copy link
Contributor

f00f commented Jan 12, 2018

@jangernert
Copy link
Collaborator Author

jangernert commented Jan 19, 2018

@f00f
New Attributes are implemented and all of RealSense2 was converted to using them. MetriCam2 doesn't do anything with them atm. I wanted to check if everything looks like it is supposed to before diving into that topic.
This is what a parameter of the type bool looks like:

[Description("Auto White Balance", "Enable / disable auto-white-balance", ConnectionStates.Connected, ConnectionStates.Connected)]
[SimpleType(typeof(bool))]
public bool AutoWhiteBalance
{
    get
    {
        CheckOptionSupported(RealSense2API.Option.ENABLE_AUTO_WHITE_BALANCE, "AutoWhiteBalance", RealSense2API.SensorName.COLOR);
        return RealSense2API.GetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.ENABLE_AUTO_WHITE_BALANCE) == 1.0f ? true : false;
    }

    set
    {
        CheckOptionSupported(RealSense2API.Option.ENABLE_AUTO_WHITE_BALANCE, "AutoWhiteBalance", RealSense2API.SensorName.COLOR);
        RealSense2API.SetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.ENABLE_AUTO_WHITE_BALANCE, value ? 1.0f : 0.0f);
    }
}

And here is a parameter which has a constraint for its range:

[Description("Gamma", "Color image gamma", ConnectionStates.Connected, ConnectionStates.Connected)]
[Range("GammaMin", "GammaMax", typeof(int))]
public int Gamma
{
    get
    {
        CheckOptionSupported(RealSense2API.Option.GAMMA, "Gamma", RealSense2API.SensorName.COLOR);
        return (int)RealSense2API.GetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.GAMMA);
    }

    set
    {
        CheckOptionSupported(RealSense2API.Option.GAMMA, "Gamma", RealSense2API.SensorName.COLOR);
        ValidateRange<int>(GammaRange, value, 0);
        RealSense2API.SetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.GAMMA, (float)value);
    }
}

public int GammaMin { get => GammaRange.Minimum; }
public int GammaMax { get => GammaRange.Maximum; }
public Range<int> GammaRange
{
    get
    {
        Range<int> res = new Range<int>(0, 0);

        if (this.IsConnected)
        {
            var option = QueryOption(RealSense2API.Option.GAMMA, RealSense2API.SensorName.COLOR);
            res = new Range<int>((int)option.min, (int)option.max);
        }
        return res;
    }
}

@jangernert jangernert linked a pull request Jan 29, 2018 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants