fix(particle): #1417 free Particle instead of Point2D in ParticleSystem.onUpdate#1480
Open
AndrewAssad2 wants to merge 2 commits into
Open
fix(particle): #1417 free Particle instead of Point2D in ParticleSystem.onUpdate#1480AndrewAssad2 wants to merge 2 commits into
AndrewAssad2 wants to merge 2 commits into
Conversation
…pdate Closes AlmasB#1417. In ParticleSystem.onUpdate, the outer destructured lambda parameter `p` (a Point2D representing the emitter position) shadowed the intended target of Pools.free. Because Pools.free is a no-op for unregistered types, Particle instances expired silently without being returned to the pool, defeating object pooling and producing GC pressure under heavy effects. Changes: - Rename the lambda parameter from `p` to `position` to remove shadowing. - Update the corresponding emitter.emit(...) call accordingly. - Replace Pools.free(p) with Pools.free(particle), so the dead Particle is recycled rather than the Point2D position. No public API changes.
Adds a JUnit 5 test in ParticleSystemTest that verifies expired Particles are returned to the Pools cache after onUpdate. The test drives a ParticleSystem through 10 update cycles (spawning and expiring 250 particles), then reflectively reads the private Pools.typePools map to assert that a Particle pool entry exists. Before the fix for AlmasB#1417, the pool would not contain a Particle entry because Pools.free was being called with a Point2D argument and silently no-opped for unregistered types. This test would fail on the previous code and passes with the fix applied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1417.
In
ParticleSystem.onUpdate, expiredParticleinstances are not returned to thePoolscache. The callPools.free(p)passes the wrong argument —pis the emitter'sPoint2Dposition (from the outer destructured lambda parameter(emitter, p)), not the deadParticle. BecausePools.freesilently does nothing for types it doesn't know about, the bug is invisible at runtime but causes the particle pool to never refill, leading to extra GC pressure when particle effects run for a long time.Where the bug is
File:
fxgl-core/src/main/kotlin/com/almasb/fxgl/particle/ParticleSystem.ktemitters.forEach { (emitter, p) -> // p : Point2D ... while (iter.hasNext()) { val particle = iter.next() // particle : Particle if (particle.update(tpf)) { iter.remove() pane.children.remove(particle.view) Pools.free(p) // bug: frees Point2D, not Particle } ... } }The same pattern in
ParticleComponent.kt(line 54) works correctly because its loop is not nested inside aforEachwhose lambda parameter is also calledp.What I changed
In
ParticleSystem.kt:Pools.free(p)toPools.free(particle)so the real Particle gets returned to the pool.ptopositionso the shadowing can't happen again.emitter.emit(p.x, p.y)toemitter.emit(position.x, position.y)so the rename is consistent.No public API changes.
Test
I added a new test
Expired particles are returned to the poolinParticleSystemTest.kt. It:ParticleSystemwith one emitter (50 particles per frame, 5 max emissions, 0.5s lifespan).onUpdate(0.5)ten times so all 250 particles spawn and die.Pools.typePoolsmap and checks that aParticlepool entry exists.Before the fix, no Particle pool ever gets registered (because
Pools.free(Point2D)does nothing), so the test would fail. After the fix it passes.Notes