Extract Review code refactor

[BigRoy]

Brief description

Trying to reduce complexity of Extract Review plug-in

• Re-use profile filtering from lib
• Remove "combination families" additional filtering which supposedly was from OP v2
• Simplify 'formatting' for filling gaps
• Use legacy_io.Session over os.environ

Testing notes:

1. Extract some reviews

[BigRoy]

I'll leave it as this for now since further cleanup/refactor might be warranted only after a check of current changes to avoid making way too big of a PR.

Pretty decent score:

[mkolar]

I'll leave it as this for now since further cleanup/refactor might be warranted only after a check of current changes to avoid making way too big of a PR.

Pretty decent score:

Should we make this into a new game? The best add/reduce ratio of a PR gets a cookie.

[iLLiCiTiT]

Merged develop, resolved few conflicts, modified few lines to match develop changes. Removed task name filtering from ExtractReview profiles -> it is not available in settings, and was not for a long time. Replaced AssertionError and ValueError with KnownPublishError. Modified hole frame mapping to more readable snipped (commit https://github.com/ynput/OpenPype/pull/3930/commits/5b7e5bff53128d509dcc9e21c07a540ed1da73c3 ).

[BigRoy]

Modified hole frame mapping to more readable snipped (commit https://github.com/ynput/OpenPype/commit/5b7e5bff53128d509dcc9e21c07a540ed1da73c3 ).

This is a different behavior however - this would be taking previous frame, not the nearest frame. If that's what we want we can even simplify this further.

It seems that this doesn't work:

col.indexes[0]
# Error: TypeError: file <maya console> line 6: 'SortedSet' object is not subscriptable

To avoid converting the whole list I'd say taking the first frame would be:

print(next(iter(col.indexes)))

        # Prepare which hole is filled with what frame
        #   - the frame is filled only with already existing frames
        prev_frame = next(iter(col.indexes)))
        hole_frame_to_nearest = {}
        for frame in range(int(start_frame), int(end_frame) + 1):
            if frame in col.indexes:
                prev_frame = frame
            else:
                # Use previous frame as source for hole
                hole_frame_to_nearest[frame] = prev_frame

The "nearest frame" logic could be simplified with min() a lot. I had actually done that previously in an earlier commit here but optimized (speed) it to what it became with more lines of code since we knew the indexes to be sorted.

The question thus becomes:

• Do we want it to be nearest frame?
• Or previous frame? (with the edge case that if first frames are missing that those would take the first existing frame)

[iLLiCiTiT]

The question thus becomes:

I would say it was actually never used, it was added for edge cases of one client. Finding nearest frame is unnecessary complex and not sure how much useful? So I vote for option 2 (previous frame). You're right it could simplified even more with min, can you commit the change?

[BigRoy]

I would say it was actually never used, it was added for edge cases of one client. Finding nearest frame is unnecessary complex and not sure how much useful? So I vote for option 2 (previous frame). You're right it could simplified even more with min, can you commit the change?

I have cleaned it up with https://github.com/ynput/OpenPype/pull/3930/commits/d13b8208f5618952362171241605d4cdd1094c04 but kept the "previous" frame logic instead of nearest frame.

I guess someone should give the PR a few test runs?