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

Steve/instance aggregation #563

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

StevenBorkman
Copy link
Contributor

Modified instance segmentation labeler to support aggregating children into a parent's instance segmentation result. This capability is built on changes made recently to labeling to support hierarchical label definitions. Now when a user requests to aggregate the results, all of the sub-components will be considered the same instance of an object. If a user selects to not aggregate their results, then each sub-component will be considered a distinct instance of an object.

Copy link
Collaborator

@mkamalza mkamalza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks! Just one required change for the compile error. I'm setting it to approve as I won't be around for a while.

/// a part of their parent object. If this value is true, the children will be reported as a part of their
/// parent.
/// </summary>
[Tooltip("Should the instance segmentation capture the single instance of the parent gameobject, or the individual sub-components")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[Tooltip("Should the instance segmentation capture the single instance of the parent gameobject, or the individual sub-components")]
[Tooltip("Should the instance segmentation capture the single instance of the parent GameObject, or the individual labelled sub-components")]

Comment on lines 137 to 141
#if UNITY_2020_1_OR_NEWER
m_LabelListView.onSelectionChange += UpdateMoveButtonState;
m_LabelListView.selectionChanged += UpdateMoveButtonState;
#else
m_LabelListView.onSelectionChanged += UpdateMoveButtonState;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives me an error on 2021.3.x. (official perception supported version). Looking at the api, it looks like selectionChanged is only supported on 2023.1 and later, do you happen to be using that one?

To make it work, we can change the whole block to

#if UNITY_2023_1_OR_NEWER
            m_LabelListView.selectionChanged +=  UpdateMoveButtonState;
#elif UNITY_2020_1_OR_NEWER
            m_LabelListView.onSelectionChange +=  UpdateMoveButtonState;
#else
            m_LabelListView.onSelectionChanged +=  UpdateMoveButtonState;
#endif

This should make the tests pass too.


if (!PerceptionCamera.savedHierarchies.TryGetValue(frame, out var hierarchyInformation))
{
Debug.LogError($"Could not get the scene hierarchy info for the current frame: {frame}");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Debug.LogError($"Could not get the scene hierarchy info for the current frame: {frame}");
Debug.LogError($"Instance segmentation could not get the scene hierarchy info for the current frame: {frame}. Will not aggregate labelled children.");

if (i > max) max = i;
}

var activeColors = new NativeArray<Color32>((int)(max + 1), Allocator.Temp, NativeArrayOptions.ClearMemory);
Copy link
Collaborator

@mkamalza mkamalza Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One little concern that I have here is that the array of instance ids can grow indefinitely in projects that use object caching, because we only remove instance ids when objects are destroyed.
Therefore, with object caching, the activeColors array can get quite big in projects that go for thousands of iterations with tens of labelled objects in each frame (like Escher and it's more advanced version Hosta).

With 1000 iterations and 20 labelled objects each iteration, activeColors could be around 80KBs. May not be a big deal, but just something to keep in mind.

// Also, this only supports a 1-1 mapping of int id to label and label to int id. Re-using labels will
// break this but right now is *probably* supported in perception. We need to revisit that and codify
// that label strings need to be unique.
var labelMap = new Dictionary<string, int>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try to cache this? We don't supporting updating of label configs at runtime any way.
Although then we'd need to implement a hash or sth to check to make sure the config is unchanged, in case we decide to support runtime config updating at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants