Skip to content

Conversation

@paulcscharf
Copy link
Member

Based on #421

This:

  • Minimizes allocations (only the scope and the list of effects are allocated, everything else lives on the stack)
  • Minimizes boilerplate (primarily by removing the phenomenon types)
  • Improve performance (and prevent boxing) by requiring IEquatable (which records implement by default)
  • Simplifies scope life cycle further and fixes bug, as suggested in 📚 Break down the status effect state machine into smaller steps #421

- Minimizes allocations
- Minimizes boilerplate
- Simplifies lifecycle slightly
@paulcscharf paulcscharf requested a review from tomrijnbeek May 9, 2025 22:36
Copy link
Member

@tomrijnbeek tomrijnbeek left a comment

Choose a reason for hiding this comment

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

I only reviewed the system files, not the implementations. I assume they will change anyway if any of the feedback changes the systems.

A bit of general feedback on the shape of this PR. This PR is big, and ultimately it's trying to do three separate things all at once:

  1. Allocation optimisation
  2. Architectural changes
  3. Style changes

The fact that this is all in one big piece makes it a bit hard to judge on. The allocation optimisation is largely non-controversial and if those had been done in a PR in isolation, would likely have been merged with little comment. The architectural changes are a bit more controversial, and are really the source of the biggest comments I had. The style choices - merging partial classes into a single file - are not really necessary. Especially since this is a PR to a PR, I feel not making stylistic changes helps avoid making the PRs more controversial than they already are.

I understand the argument for merging things into a single PR, but it would've been nice to see the work at least split out in commits, so that:

  1. It's easier to review the different changes in isolation
  2. It's easier to cherrypick the changes we want if we agree on the non-controversial stuff but disagree on the controversial topics

@tomrijnbeek tomrijnbeek merged commit 3829e2c into better-status-effects May 12, 2025
@tomrijnbeek tomrijnbeek deleted the better-status-effects-2 branch May 12, 2025 19:23
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.

3 participants