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

Only log Clears when needed in face tracking example #4950

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 29, 2024

This is what caused the deadlocks/crashes we were after (which I'm glad it did -- those have been fixed in #4949).

Even though they are not a problem anymore, I'm not sure why they're here to begin with? Clearing scalars does nothing unless you specifically intend to use them with latest-at queries (i.e. not plots, which isn't the case here), otherwise it's just inefficient with no upside (which I'd rather not teach users). Am I missing something? 🤔

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added examples Issues relating to the Rerun examples 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 29, 2024
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

I'm not convinced about this change. It would yield weird result when face(s) are no longer detected.

@@ -239,7 +239,6 @@ def detect_and_log(self, image: npt.NDArray[np.uint8], frame_time_nano: int) ->
)
rr.log("video/landmarker/faces", rr.Clear(recursive=True))
rr.log("reconstruction/faces", rr.Clear(recursive=True))
rr.log("blendshapes", rr.Clear(recursive=True))

for i, (landmark, blendshapes) in enumerate(
zip(detection_result.face_landmarks, detection_result.face_blendshapes)
Copy link
Member

Choose a reason for hiding this comment

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

Those clear happen because the number of face detected can vary from 0 to N. That's why we clear faces/** and the re-populate faces/$i/**. Maybe a cleaner way to go about this would be to detect when faces disappear and clear only those ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the intent here.
Clearing effectively doesn't affect range queries at all, so what is the expected behavior we should expect? I don't get it.

I.e. this:

for step in range(0, 64):
    rr.set_time_sequence("step", step)
    rr.log("scalar", rr.TimeSeriesScalar(math.sin(step / 10.0)))

https://app.rerun.io/pr/4949/index.html?url=https://storage.googleapis.com/rerun-builds/pull_request/4950/scalar_no_clears.rrd

results in the same exact graph as this:

for step in range(0, 64):
    rr.set_time_sequence("step", step)
    rr.log("scalar", rr.TimeSeriesScalar(math.sin(step / 10.0)))
    rr.log("scalar", rr.Clear(recursive=True))

https://app.rerun.io/pr/4949/index.html?url=https://storage.googleapis.com/rerun-builds/pull_request/4950/scalar_clears.rrd

Copy link
Member

Choose a reason for hiding this comment

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

You're right, not clearing here as no effect, so it's ok to remove it (but I'd leave a TODO to explain why this one is not cleared while the others are).

Incidentally, it would be really nice if those Clears would actually translate into breaks in the timeseries to avoid those ugly jumps:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Haaa interesting! Let me see if there's something quick we can do about this.

@teh-cmc teh-cmc force-pushed the cmc/why_clear_scalars_tho branch from 4acdc98 to 1a7ce62 Compare January 30, 2024 08:15
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 30, 2024

I've updated the code to only clear faces when they disappear.

I might come up with a follow up PR soon so that the timeseries view interprets those clears as plot breakpoints.

@abey79
Copy link
Member

abey79 commented Jan 30, 2024

Nice! Clear == break would actually be a very nice improvement tying nicely into the plotting story.

@teh-cmc teh-cmc merged commit 2c60146 into main Jan 30, 2024
40 checks passed
@teh-cmc teh-cmc deleted the cmc/why_clear_scalars_tho branch January 30, 2024 08:38
@teh-cmc teh-cmc changed the title Don't clear scalars in face tracking example Only log Clears when needed in face tracking example Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Issues relating to the Rerun examples exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants