Skip to content

Re-wrote feathers example to use less stack. #20645

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Re-wrote feathers example to use less stack. #20645

wants to merge 2 commits into from
+149 −136

Conversation

viridia
Copy link
Contributor

@viridia viridia commented Aug 18, 2025

Objective

Fixes #20571

Solution

Used SpawnWith to lazily spawn some entities.

Testing

I tested this manually, but as I am unable to reproduce the bug myself I hope someone will be able to verify it for me.

@james7132 james7132 added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples A-UI Graphical user interfaces, styles, layouts, and widgets O-Windows Specific to the Windows desktop operating system labels Aug 19, 2025
@james7132 james7132 added this to the 0.17 milestone Aug 19, 2025
james7132
james7132 approved these changes Aug 19, 2025
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I just locally tested this on my Windows 11 machine and main stack overflowed, while this ran without issue.

james7132
james7132 reviewed Aug 19, 2025
View reviewed changes
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 19, 2025
@mnmaita
Copy link
Member

mnmaita commented Aug 19, 2025

@viridia aside the overflow fix, this looks much better! Should be ready to go after running the formatter IMO.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Aug 19, 2025
@alice-i-cecile
Copy link
Member

Blocking on design discussion in #20571: I am very reluctant to simply mask the problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

+1 more reviewer

@mnmaita mnmaita mnmaita approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples O-Windows Specific to the Windows desktop operating system S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Stack overflow with large-ish Bundle spawn
4 participants
@viridia @mnmaita @alice-i-cecile @james7132