Skip to content

Conversation

@NicolasHug
Copy link
Contributor

@NicolasHug NicolasHug commented Oct 25, 2024

This is about exposing the 2 C++ methods from #280 and #282 into the Python class.

It feels natural to want these methods to be called get_frames_at() and get_frames_displayed_at(), but these names are already occupied by our "range" function. We may then need to rename these existing "range" functions (see diff).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 25, 2024
@scotts
Copy link
Contributor

scotts commented Oct 25, 2024

I was puzzling over this myself, and I like this solution. I had considered keeping all current APIs as-is, and adding get_frames_at_indices() and get_frames_displayed_at_timestamps(), which is clunkier. This is cleaner.

@ahmadsharif1
Copy link
Contributor

This is not my first preference, but you can merge it.

Reason is that "displayed" is not the critical difference b/w the two methods. Frames are always displayed -- whether at an index or timestamp.

I would have picked get_frame_at_index and get_frame_at_time, get_frames_at_index_range, get_frames_at_time_range, etc.

I like names that you can read from the callsite to tell what parameters they would need and what they are doing and other people may disagree with that.

@NicolasHug
Copy link
Contributor Author

Thank for the feedback. I'll follow-up with separate PRs.

@NicolasHug NicolasHug closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants