Skip to content

Conversation

@MustafaMulla29
Copy link
Contributor

@MustafaMulla29 MustafaMulla29 commented Mar 17, 2025

/claim #42

obj_limit.mp4

@vercel
Copy link

vercel bot commented Mar 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
graphics-debug ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 17, 2025 5:16pm

return (
<InteractiveGraphics
objectLimit={2}
graphics={{
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont break an existing fixture just to test something new, create a new fixture here

Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

id like to avoid consolidating into a single array then expanding out again (it’s worse for performance which is a big issue atm, and it makes the code harder to understand)

Youre also biasing the limit to whatever is appended to the array first.

i think we should actually reverse bias, we should show the last items of the array if we exceed the limit, because this is what is most recently generated for most debug visualizations

@MustafaMulla29 MustafaMulla29 requested a review from seveibar March 17, 2025 16:33
@MustafaMulla29
Copy link
Contributor Author

@seveibar how's it now?(correct me if something's wrong)

@seveibar
Copy link
Contributor

@MustafaMulla29 still haven't addressed my feedback, e.g. with consolidation of objects

@MustafaMulla29
Copy link
Contributor Author

@seveibar Now can you check please. Now it's mapping each object seperately

Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

originalIndex has to be preserved to make sure things are rendered properly when the graphics object changes

@MustafaMulla29 MustafaMulla29 requested a review from seveibar March 17, 2025 17:16
Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

technically, objectLimit is objectLimitPerCategory given your implementation, in most cases where there are both lines + points etc. there will be 4x objectLimit present. So this isn't correct but it might be good enough

i'm not sure if the shallow cloning will have a performance implication, you are sort of shallow copying then slice-removing a bunch of entries so the shallow clone wasn't needed. But it might not matter

@seveibar seveibar merged commit e3da1f6 into tscircuit:main Mar 17, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants