[DO NOT MERGE] fix(controller): prevent Re-ID fallback from firing while features are still being gathered

[Copilot]

📝 Description

In UUIDManager.assignID, the conditional flow was restructured to prevent the GID fallback from firing while Re-ID feature collection is still in progress.

Previously, the fallback branch (which assigns the generated GID to active_ids) ran not only when Re-ID is disabled, but also during the first ~DEFAULT_MINIMUM_FEATURE_COUNT frames of every new track while features were still accumulating. Once active_ids[rv_id][0] was set to a non-None value, isNewTrackerID() returned False, skipping the entire new-track branch on subsequent frames — so gatherQualityVisualFeatures was never called again, haveSufficientVisualFeatures never became True, and the similarity query was never submitted. Re-ID was silently broken for all newly detected tracks.

Fix: The if/else block in assignID was restructured so that:

• When Re-ID is enabled, the similarity query is submitted only after sufficient features have been collected; no fallback fires while features are still accumulating.
• When Re-ID is explicitly disabled, the GID fallback fires immediately.

# Before — fallback fires when reid is enabled but features not yet gathered, breaking Re-ID
if self.haveSufficientVisualFeatures(sscape_object) and self.reid_enabled:
    ...
else:
    # prematurely locks in the GID, preventing further feature gathering
    self.active_ids[sscape_object.rv_id] = [sscape_object.gid, None]

# After — conditions reordered; fallback only when Re-ID is actually off
if self.reid_enabled:
    if self.haveSufficientVisualFeatures(sscape_object):
        ...
else:
    # Re-ID is disabled; fall back to using the generated GID
    self.active_ids[sscape_object.rv_id] = [sscape_object.gid, None]

Tests: 16 unit tests added in tests/sscape_tests/uuid_manager/ and wired into the BAT unit-tests target via uuid-manager-unit in tests/Makefile.sscape and tests/Makefile, covering:

• PR fix(controller): preserve UUID for static objects after occlusion (#1152) #1155 – static UUID preservation after occlusion: pruneInactiveTracks correctly accepts a set of all active C++ tracker objects (reliable + unreliable + suspended), preserving GIDs for occluded/suspended objects; pickBestID restores stored GIDs so reappearing static objects keep their original identity.
• Re-ID fallback fix: regression test confirming active_ids[rv_id] stays [None, None] while features are still accumulating; fallback fires only when Re-ID is explicitly disabled; similarity query is submitted exactly once when sufficient features are collected.

✨ Type of Change

• 🐞 Bug fix – Non-breaking change which fixes an issue
• 🚀 New feature – Non-breaking change which adds functionality
• 🔨 Refactor – Non-breaking change which refactors the code base
• 💥 Breaking change – Changes that break existing functionality
• 📚 Documentation update
• 🔒 Security update
• 🧪 Tests
• 🚂 CI

🧪 Testing Scenarios

• ✅ Tested manually
• 🤖 Ran automated end-to-end tests

✅ Checklist

• 🔍 PR title is clear and descriptive
• 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
• 💬 I have commented my code, especially in hard-to-understand areas
• 📄 I have made corresponding changes to the documentation
• ✅ I have added tests that prove my fix is effective or my feature works

[Copilot]

Pull request overview

Fixes a logic bug in UUIDManager.assignID where the “fallback” assignment of the generated GID could run while Re-ID feature collection was still in progress, permanently preventing similarity queries for newly detected tracks.

Changes:

• Guarded the GID fallback so it only executes when reid_enabled is explicitly disabled.
• Reworked the conditional flow so Re-ID similarity queries are submitted only when Re-ID is enabled and sufficient visual features have been collected.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[saratpoluri]

@copilot Add tests to this PR to check the functionality of #1155 and specific test case for the fix in this PR.

[Copilot]

@copilot Add tests to this PR to check the functionality of #1155 and specific test case for the fix in this PR.

Added 16 unit tests in tests/sscape_tests/uuid_manager/ (commit a68e928):

PR #1155 – static UUID preservation after occlusion (TestPruneInactiveTracks / TestPickBestID)

• test_preserves_tracks_present_in_active_set – track IDs in the active set are kept in active_ids
• test_removes_tracks_absent_from_active_set – IDs not in the set are pruned
• test_accepts_set_containing_suspended_tracks – passing a mixed set of reliable + unreliable + suspended tracks preserves all their GIDs (mirrors the all_active_tracks set change)
• test_suspended_track_uuid_survives_occlusion – end-to-end: a suspended track's GID survives a prune cycle
• test_increments_unique_id_count_for_unmatched_tracks / test_does_not_increment_unique_id_count_for_matched_tracks – counter semantics for matched vs unmatched departures
• test_restores_gid_from_active_ids – pickBestID restores the stored GID when a reappearing object is processed
• test_sets_similarity_none_when_no_mapping / test_accumulates_reid_vectors_for_known_track – pickBestID edge cases

Current PR fix – elif not self.reid_enabled (TestAssignIDFallbackBehavior)

• test_reid_enabled_insufficient_features_does_not_lock_gid – core regression test: active_ids[rv_id] stays [None, None] while features are still accumulating (the pre-fix else branch would lock the GID here)
• test_reid_enabled_gid_not_locked_across_multiple_frames – holds across repeated frames before the threshold is reached
• test_reid_disabled_locks_gid_immediately – fallback fires only when Re-ID is explicitly off
• test_reid_enabled_sufficient_features_submits_query / test_reid_enabled_sufficient_features_query_submitted_only_once – similarity query is submitted exactly once when sufficient features exist
• test_assign_id_known_tracker_id_calls_pick_best_id – known tracker IDs go through pickBestID
• test_reid_disabled_no_query_submitted – no DB query when Re-ID is off

[tdorauintc]

Shouldn't we add uuid-manager-unit target and recipe in tests/Makefile.sscape? And the sub-target in _unit-tests target of tests/Makefile?
The uuid tests weren't executed as part of unit-test target when I ran it locally.

[tdorauintc]

Minor issue: PR fix description is not up-to-date with the implementation. PR description mentions Fix: change else → elif not self.reid_enabled, while the actual fix reorders the if conditions.

[tdorauintc]

I got the following errors after adding the targets for uuid-manager tests following the convention used for other unit tests. Please provide the correct way to run the tests and make sure that those are executed as a part of BAT.

==================================== ERRORS ====================================
______________ ERROR collecting uuid_manager/test_uuid_manager.py ______________
ImportError while importing test module '/home/tdorau/repos/scenescape/tests/sscape_tests/uuid_manager/test_uuid_manager.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/sscape_tests/uuid_manager/test_uuid_manager.py:17: in <module>
    from controller.uuid_manager import (DEFAULT_MINIMUM_FEATURE_COUNT,
E   ModuleNotFoundError: No module named 'controller.uuid_manager'
- generated xml file: /home/tdorau/repos/scenescape/tests/reports/test_reports/unit-tests/uuid-unit.xml -
=========================== short test summary info ============================
ERROR tests/sscape_tests/uuid_manager/test_uuid_manager.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.22s ===============================
/usr/local/lib/python3.10/dist-packages/coverage/inorout.py:561: CoverageWarning: Module sscape/ was never imported. (module-not-imported); see https://coverage.readthedocs.io/en/7.14.0/messages.html#warning-module-not-imported
  self.warn(f"Module {pkg} was never imported.", slug="module-not-imported")
/usr/local/lib/python3.10/dist-packages/coverage/control.py:958: CoverageWarning: No data was collected. (no-data-collected); see https://coverage.readthedocs.io/en/7.14.0/messages.html#warning-no-data-collected
  self._warn("No data was collected.", slug="no-data-collected")

[saratpoluri]

@copilot address code review comments.

[Copilot]

@copilot address code review comments.

Addressed in commit 8ca275b:

• Added uuid-manager-unit target to tests/Makefile.sscape (uses $(IMAGE)-controller-test, alphabetical order between transform-unit and views-unit)
• Wired uuid-manager-unit into the _unit-tests prerequisite list in tests/Makefile so it runs as part of the BAT unit-tests target

The controller-test image is used because the tests import from controller.uuid_manager (same as scene-unit, mesh-util-unit, and robot-vision-unit).

[tdorauintc]

I will re-review once the PR is rebased onto refactored tests and all checks are passing.