NVlabs / FPSci

Aim Training Experiments
Other
70 stars 23 forks source link

Target color configuration #285

Closed bboudaoud-nv closed 3 years ago

bboudaoud-nv commented 3 years ago

This branch adds support for a colors field for targets in the target array of the experiment config. The field takes an array of 2 Color3 elements to set the (linearly interpolated) max/min target health colors. If specified these colors override a setting in the experiment/session config targetHealthColors field value. If not specified these colors fall back to the (default) targetHealthColors.

Merging this PR closes #266.

jspjutNV commented 3 years ago

Thinking about this, I think I'd expect the opposite priority from what is described here, and I want to have some discussion before we merge this. In my mind, if I set something in the session, I'd expect it to override whatever may have been set anywhere else. That is the global target list is the defaults, and I'd expect to override those defaults by whatever is set in the session.

Since I think we need to discuss this decision, and this feature isn't really urgently needed for anything I'm aware of, I'm also going to defer this PR for the next release.

bboudaoud-nv commented 3 years ago

@jspjutNV your reasoning here makes sense, although the inclusion of targets in trials is also part of the session specification as well. I agree we can defer this issue for the next release (no direct ask for this feature).

My primary issue with the session-level parameter overriding the per-target specification is that it eliminates the ability to have different color targets when specified (i.e. if specified all targets must be this color). Also specifying this field at the experiment level would inherently specify it at the session level. Certainly something to discuss more.

jspjutNV commented 3 years ago

I tried a couple different implementations to get more than 2 colors working, and I got a couple that work, but I think are pretty ugly code-wise. Instead of creating 2-3 pull requests with the different variant implementations, I'm just going to put the code here. All of these implementations would replace the code in the else { block in FPSciApp::lerpColor.

First, this one is best commented, but pretty ugly.

        // For 2 or more colors, linearly interpolate between the N colors.
        // a comes in the range [0, 1] where 
        //     0 means to use the last value in colors
        // and 1 means to use the first value in colors
        // This means that a = 0 maps to colors[colors.length() - 1]
        // and a = 1 maps to colors[0]
        // We need to flip the direction and scale up to the number of elements in the array
        // a will indicate which two entries to interpolate between (left of .), and how much of each to use (right of .)
        float interp = (1.0f - a) * (colors.length() - 1);
        int idx = int(floor(interp));
        interp = interp - float(idx);
        Color3 output = colors[idx] * (1.0f - interp) + colors[idx + 1] * interp;
        //debugPrintf("\tcolor = (%.3f, %.3f, %.3f)\n", output.r, output.g, output.b);
        return output;

Next is the one I got to experimentally, but doesn't have quite as nice comments explaining it.

        // For 2 or more colors, just interpolate between the first 2
        a = a * (colors.length() - 1);
        int i = int(floor(a));
        a = a - float(i);
        i = colors.length() - 1 - i;
        Color3 output = colors[i - 1] * a + colors[i] * (1.0f - a);
        //debugPrintf("\tcolor = (%.3f, %.3f, %.3f)\n", output.r, output.g, output.b);
        return output;

And finally is the implementation that doesn't quite work yet, but seems cleanest if we can make it work. Again, the below has a bug in it that I haven't fixed yet.

                // Does not work (yet)
        int i = colors.length() - 1;
        a = a * i;
        while (a >= 1) {
            a -= 1.0f;
            i -= 1;
        }
        Color3 output = colors[i] * a + colors[i + 1] * (1.0f - a);
        //debugPrintf("\tcolor = (%.3f, %.3f, %.3f)\n", output.r, output.g, output.b);
        return output;

Again, none of these is particularly pleasing to me visually, but I figured it was worth saving the progress in case we ever wanted to allow for more than 2 color end points for the color gradient.

We'd also want to document the functionality, and at the moment, it looks like it works by creating 10 points in the range color[1] to color[0] but excludes the final point. In other words, you get the following colors.

color[1]    color[0]    Target life range
100%        0%          0-10%
90%         10%         10-20%
80%         20%         20-30%
70%         30%         30-40%
60%         40%         40-50%
50%         50%         50-60%
40%         60%         60-70%
30%         70%         70-80%
20%         80%         80-90%
10%         90%         90-100%
jspjutNV commented 3 years ago

Thinking a bit more about it, I'm not sure if the current functionality (based on the table from my last post) is what we want. It seems a bit weird to me that we only exactly represent one of the specified colors when 2 colors are present. When 3 or more colors are provided, there are various implementations that would make sense, and it seems sensible to me that the designer would want to see the colors they specify in the list actually show up. This would greatly complicate the implementation since we'd want to snap a specified color to a particular target health percentage, ideally as close to evenly spaced as possible. In the limit, to get 10 exact colors, the experiment config would need to specify 11 colors, the last of which isn't used at all.

We could potentially work around this by allowing the experiment config to specify the number of color levels, though that opens a whole can of worms I'm not too interested in thinking through right now.

I'd be interested in hearing any arguments for or against the current number of steps of 10, and I'd be tempted to suggest 12 levels (meaning 13 total colors, 1 throw away) as a way to have more common factors for small color counts. Example table below. The way to read this is the first row a Color ID appears is when that color is 100% of the output, and subsequent numbers are equally divided linear interpolation toward the next full color.

Target %     | Colors Provided
             | 1 2 3 4 5 7 13
-------------|-----------------
0-8.33%      | 1 1 1 1 1 1  1
8.34-16.67%  | 1 1 1 1 1 1  2
16.67-25%    | 1 1 1 1 1 2  3
25-33.33%    | 1 1 1 1 2 2  4
33.33-41.67% | 1 1 1 2 2 3  5
41.67-50%    | 1 1 1 2 2 3  6
50-58.33%    | 1 1 2 2 3 4  7
58.33-66.67% | 1 1 2 2 3 4  8
66.67-75%    | 1 1 2 3 3 5  9
75-83.33%    | 1 1 2 3 4 5 10
83.33-91.67% | 1 1 2 3 4 6 11
91.67-100%   | 1 1 2 3 4 6 12
bboudaoud-nv commented 3 years ago

I've added a method to make the target materials from a target config and with it added a fix that makes sure the "full health" color is accurate represented (it wasn't previously). I believe we can likely fix the remainder of the issues noted above with improved documentation of whatever behavior we decide to implement.

bboudaoud-nv commented 3 years ago

We should consider when/where to (potentially) clear the FPSciApp::materials table before closing this PR. Currently this table is never cleared.

jspjutNV commented 3 years ago

We should consider when/where to (potentially) clear the FPSciApp::materials table before closing this PR. Currently this table is never cleared.

I'm pushing something that will remove the previous list per "id" when we set a new one, but I'll note that we don't clear targetModels or m_explosionModels either, and all of these will likely be memory leaks from long running FPSci usage with lots of session and experiment switching. I'll open an issue to track this likely problem.