PacMan effect #4891
PacMan effect #4891
Conversation
WalkthroughAdds a Pac‑Man effect: defines FX_MODE_PACMAN (ID 151), implements static mode_pacman with a pacmancharacters_t state, adds _data_FX_MODE_PACMAN PROGMEM metadata, allocates per‑segment state for characters and power‑dots, renders Pac‑Man/ghosts/dots, and registers the effect (registration appears twice). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wled00/FX.h (1)
310-310: Nit: strengthen the inline comment to avoid SR-ID confusion.The block header says “SR compatible IDs,” while this line is non-SR. Clarify in-line to discourage future renumbering.
Apply this tiny comment tweak:
-#define FX_MODE_PACMAN 151 // gap fill - not SR +#define FX_MODE_PACMAN 151 // gap fill (non-SR). Do NOT renumber; SR-ID range must remain stable.wled00/FX.cpp (3)
3291-3294: Power-dot rendering is safe, but keep positions guarded by contractCurrent positions come from controlled math and remain within [0, SEGLEN-1]. If future changes alter placement logic, consider mirroring the same bounds check pattern used for characters to keep this robust.
3170-3177: Prefer explicit integer widths for positions
signedforpos/topPosworks but is vague. Useint16_t(orint32_tif needed) to make intent and size explicit and minimize memory.-typedef struct PacManChars { - signed pos; - signed topPos; +typedef struct PacManChars { + int16_t pos; + int16_t topPos;
3338-3341: Decouple movement from PacMan’s direction (optional)Ghost movement currently keys off
character[PACMAN].direction. Using each ghost’s owndirectionimproves clarity and future tweaks (e.g., different behaviors per ghost) with zero behavior change today.- character[i].pos += character[PACMAN].direction?1:-1; + character[i].pos += character[i].direction ? 1 : -1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.hwled00/FX.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.hwled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
🔇 Additional comments (3)
wled00/FX.h (1)
310-310: Approved: FX_MODE_PACMAN addition verified
Unique define at 151, single addEffect registration in FX.cpp, and MODE_COUNT remains 218.wled00/FX.cpp (2)
3312-3331: Reset logic depends solely on the end-of-strip power dotThe eaten-state reset after returning to LED 0 triggers only if
character[5](the end-of-segment dot) was eaten. If you intended “eat any power dot → chase → on return to start reset all,” this condition can leave dots eaten if the end dot was skipped. Verify intended game flow.Would you like me to adjust the reset to trigger if any power dot was eaten during the chase window?
10980-10981: Registration confirmed for FX_MODE_PACMAN
FX_MODE_PACMANis defined once (FX.h:310), its data is declared once (FX.cpp:3356), and it’s registered exactly once via addEffect (FX.cpp:10980).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
wled00/FX.cpp (2)
3249-3256: Guard the i-1 write when drawing spaced white dotsWhen Compact Dots is off, add a simple bound to avoid an i-1 underflow if this loop ever reaches 0 after future tweaks.
Apply:
- for (int i = SEGLEN-1; i >= character[PACMAN].topPos+1; i--) { + for (int i = SEGLEN-1; i >= character[PACMAN].topPos+1; i--) { SEGMENT.setPixelColor(i, WHITEISH); if (!SEGMENT.check2) { - SEGMENT.setPixelColor(i-1, BLACK); - i--; + if (i > 0) { + SEGMENT.setPixelColor(i-1, BLACK); + i--; + } } }
3290-3294: Bounds-guard power-dot drawsPower-dot positions are computed, but a lightweight guard is safer against future changes and prevents accidental OOB writes.
Apply:
- for (int i = 0; i < numPowerDots; i++) { - if (!character[i+5].eaten) - SEGMENT.setPixelColor(character[i+5].pos, character[i+5].color); - } + for (int i = 0; i < numPowerDots; i++) { + if (!character[i+5].eaten) { + if ((unsigned)character[i+5].pos < SEGLEN) + SEGMENT.setPixelColor(character[i+5].pos, character[i+5].color); + } + }
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
3335-3352: Simplify in-range checksThe “>= 0” part is redundant after casting to unsigned; keeping only “< SEGLEN” is clearer.
Apply:
- if ((unsigned)character[PACMAN].pos >= 0 && (unsigned)character[PACMAN].pos < SEGLEN) + if ((unsigned)character[PACMAN].pos < SEGLEN) SEGMENT.setPixelColor(character[PACMAN].pos, character[PACMAN].color); @@ - if ((unsigned)character[i].pos >= 0 && (unsigned)character[i].pos < SEGLEN) + if ((unsigned)character[i].pos < SEGLEN) SEGMENT.setPixelColor(character[i].pos, character[i].color);Do the same in the “no update” branch just below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-08-31T03:38:14.092Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
🔇 Additional comments (2)
wled00/FX.cpp (2)
3170-3177: Good call keeping pos/topPos as signed intsMatches maintainer guidance and avoids needless overhead.
10984-10985: Registration looks correct (no duplicate)Effect is registered once with proper metadata. Nothing else to do here.
|
Modified the latest batch of suggestions by CodeRabbit. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
📚 Learning: 2025-08-31T03:38:14.092Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
🔇 Additional comments (7)
wled00/FX.cpp (7)
3170-3177: Good call keeping positions as signed ints (per maintainer guidance).Using signed ints for pos/topPos avoids unnecessary casts and overhead. Matches the preference noted by blazoncek.
3189-3192: Nice UX: reinit when power‑dot count changes.Resetting on aux0 change keeps state consistent when the slider is moved.
3248-3257: Bounds-safe compact dots.Guarding the i-1 write prevents underflow at index 0. Thanks for tightening this.
3292-3298: Power-dot draw uses in-bounds guard.Casting pos to unsigned and checking < SEGLEN avoids OOB writes when dots are off-segment. Good.
3339-3357: Character draws now bounded.The in-range checks around Pac‑Man and ghost positions prevent buffer corruption on transient -1/SEGLEN positions.
3364-3365: Effect metadata looks consistent.Sliders/checks map correctly; default o1=1 (White dots enabled) matches the intended look post-gamma changes.
10888-10889: Pac-Man effect is only registered once. Search found a singleaddEffect(FX_MODE_PACMAN, …)call at FX.cpp:10988.
| pacmancharacters_t *character = reinterpret_cast<pacmancharacters_t *>(SEGENV.data); | ||
|
|
||
| unsigned startBlinkingGhostsLED; // the first LED when the blue ghosts will start blinking | ||
| if (SEGLEN > 150) |
There was a problem hiding this comment.
does this still look ok to you with 1000 or 2000 LEDs?
| startBlinkingGhostsLED = SEGLEN/3; // for short strips, start blinking the ghosts when there is 1/3rd of the LEDs left | ||
|
|
||
| if (SEGENV.call == 0) { | ||
| for (int i = 0; i < 5; i++) { |
This was already reviewed by dedahai and blazoncek in PR #4536, but that one had other code in it for another effect and we are not including that other effect in this PR. That other PR was a draft and has been closed.
The only PacMan code difference between that PR and this one is that I had to modify the colors of the characters and powerdots because of gamma correction fixes in the last few months. They made the character colors change enough where they didn't look good anymore. So colors are back to where they should be if Gamma Correction is enabled.
The other effect in the old PR is called Ants, but it will be done separately (either a normal effect or a usermod effect).
Summary by CodeRabbit