Skip to content

428 dynamic bounds margin#467

Merged
wouterlucas merged 12 commits intomainfrom
428-dynamic-bounds-margin
Feb 21, 2025
Merged

428 dynamic bounds margin#467
wouterlucas merged 12 commits intomainfrom
428-dynamic-bounds-margin

Conversation

@Drulokia
Copy link
Collaborator

Added option to set boundsmargin on node level.
Also able to change it after creation.

@Drulokia Drulokia linked an issue Dec 10, 2024 that may be closed by this pull request
Copy link
Collaborator

@jfboeve jfboeve left a comment

Choose a reason for hiding this comment

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

Hey I went through the code the initial work looks good, however I've found some points I would like to see added / changed:

For the Stage.ts:

  • Set the default boundsMargin of the stage when creating the rootNode.
  • Add getter/setter for boundsMargin on Stage and have it also adjust the boundsMargin of the rootNode.

For the CoreNode:

  • for get boundsMargin I don't think we need to check if the boundsMargin in the props is an Array or not, just check if this.props.boundsMargin is not null and return that value. If it is null let it check the parent node(s) with the changes to Stage we won't need the stage.boundsMargin
  • In case the boundsMargin for a CoreNode is set upon creating the node we should call the boundsMargin setter like we are doing with the shaders/texture (line: 761)

Copy link
Contributor

@wouterlucas wouterlucas left a comment

Choose a reason for hiding this comment

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

LGTM

@Drulokia
Copy link
Collaborator Author

Drulokia commented Jan 6, 2025

Added dynamic boundsmargin option to stage, also boundsmargin can be set to null, so it can take over parents boundsmargin or stages boundsmargin.

Went throught the changes requested by @jfboeve

@Drulokia Drulokia requested a review from jfboeve January 6, 2025 16:52
@wouterlucas
Copy link
Contributor

Added dynamic boundsmargin option to stage, also boundsmargin can be set to null, so it can take over parents boundsmargin or stages boundsmargin.

Went throught the changes requested by @jfboeve

looks good, one Q though: should we really introduce a new updateType? The RenderBound could also update its children, which might be a better pattern overall.

Currently the same behaviour exists on RenderBound whenever clipping is enabled or at the global position recalculation when clipping is enabled.

Basically this:
https://github.com/lightning-js/renderer/pull/467/files#diff-b56e593f818388b36e7083093c5bfdb07817d6b25e2aeac913c5dfb3d9cedef7R1054-R1060

Can be replaced by using UpdateType.RenderBounds if we move this:
https://github.com/lightning-js/renderer/pull/467/files#diff-b56e593f818388b36e7083093c5bfdb07817d6b25e2aeac913c5dfb3d9cedef7R1049-R1050

and this:
https://github.com/lightning-js/renderer/pull/467/files#diff-b56e593f818388b36e7083093c5bfdb07817d6b25e2aeac913c5dfb3d9cedef7R1102

to behave the same as your suggested BoundsMargin by replicating the same inside the RenderBound updateType here:
https://github.com/lightning-js/renderer/pull/467/files#diff-b56e593f818388b36e7083093c5bfdb07817d6b25e2aeac913c5dfb3d9cedef7R1062

@jfboeve
Copy link
Collaborator

jfboeve commented Jan 8, 2025

I agree with you @wouterlucas since the renderBounds are calculated based on boundsMargin it's better to add it to the RenderBound

…nly the first) And use renderBounds updateType
@Drulokia
Copy link
Collaborator Author

Drulokia commented Jan 8, 2025

I had it seperate because the renderBounds did only propagate 1 child up.
I checked if it had impact if it did all children and it doesn't.
Changed it to be handled in renderBounds UpdateType. Now renderBounds goes through all children.

Copy link
Contributor

@wouterlucas wouterlucas left a comment

Choose a reason for hiding this comment

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

Thanks!

@wouterlucas
Copy link
Contributor

can you rebase it @Drulokia ?

@wouterlucas
Copy link
Contributor

I think you had a merge issue with the unit tests, seems broken after the rebase/merge with main:
https://github.com/lightning-js/renderer/actions/runs/12904794534/job/35982692173?pr=467#step:8:30

@wouterlucas wouterlucas merged commit b21ba6d into main Feb 21, 2025
1 of 2 checks passed
@wouterlucas wouterlucas deleted the 428-dynamic-bounds-margin branch February 21, 2025 12:37
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.

Dynamic bounds margin

3 participants