Fix #8575: Correct builtin global accessors in p5.strands instance mode#8600
Fix #8575: Correct builtin global accessors in p5.strands instance mode#8600saurabh24thakur wants to merge 4 commits intoprocessing:dev-2.0from
Conversation
…rrors, and FES mocking in browser environment
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Hi! I think the implementation is more complicated than I would expect it to be, would you mind explaining a bit if you tried passing in fn and p5.Graphics.prototype and what issues that ran into?
Also what was the issue with infinite looping? Do you have a test sketch where that was happening in v2.2.2?
| }); | ||
| const id = DAG.getOrCreateNode(dag, nodeData); | ||
| CFG.recordInBasicBlock(cfg, cfg.currentBlock, id); | ||
| return { id, dimension: properties.length, components: structTypeInfo.components }; |
There was a problem hiding this comment.
What was this part fixing?
src/strands/strands_api.js
Outdated
| // Find the original descriptor if it exists on this target or its prototype chain | ||
| let originalDescriptor = null; | ||
| let curr = target; | ||
| while (curr && !originalDescriptor) { |
There was a problem hiding this comment.
Could you explain a bit why this while loop was necessary? Was p5.Graphics.prototype or fn not working correctly?
…or components Summary of changes: - Simplified installBuiltinGlobalAccessors by removing prototype chain loop. - Moved accessor installation to initGlobalStrandsAPI for earlier availability. - Fixed invalid reference to structTypeInfo.components in ir_builders.js and added clarifying comment. - Added unit test in p5.Shader.js to verify mouseX getter on p5.prototype.
|
@davepagurek I have fix those too ..Let verify my PR.. |
|
Hi @saurabh24thakur. Thanks for your time and effort on this. I've reviewed the updates and unfortunately there are still quite some puzzling parts in the code, making surprising changes that are not clear from original issue scope. You also have not answered @davepagurek's requests for explanations. Engaging constructively in discussion is a very important part of open source. To avoid blocking the project, I’m going to unassign this work for now, and open this to other volunteers. Thanks so much for your understanding. If you would like to work on either this or another task, please be sure to understand the issue more deeply and to engage with maintainers and stewards reviews. |
|
I completely understand the decision to unassign me, and I apologize for not engaging constructively with your questions earlier. I was over-complicating the solution to handle recursion issues I hit during testing, but I realize now that my lack of explanation made the code difficult to review. I have since gone back to the basics and simplified the logic to focus strictly on passing p5.prototype and p5.Graphics.prototype as targets, as @davepagurek suggested. I would appreciate one more chance to tackle this issue; I am committed to discussing the implementation here in the comments before opening a new PR to ensure it aligns with the project's standards.. |
3 participants
Resolves #8575
Changes
mouseX,width,height,deltaTime) are now correctly installed on the p5 instance andGraphics.prototype, ensuring they are recognized asStrandsNodeobjects in instance mode.this.deltaTime = ...), maintaining compatibility with p5's frame timing logic.Screenshots of the change
Manual verification confirmed:
✅ SUCCESS: p.mouseX is a StrandsNode!and resolved previous console errors (Maximum call stack size exceededandTypeError: Cannot set property deltaTime).PR Checklist
npm run lintpasses