-
-
Couldn't load subscription status.
- Fork 3.6k
Stabilize behavior of createVector() with zero arguments #8203
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.
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
base: dev-2.0
Are you sure you want to change the base?
Conversation
src/core/environment.js
Outdated
| const matrix = this._renderer.getWorldToScreenMatrix(); | ||
|
|
||
| if (screenPosition.dimensions === 2) { | ||
| if (origDimension === 2 || screenToWorldArgs === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is, when we are padding 0's to the other components of vector to prevent NaN, the screenToWorld breaks because it the count here changes and maybe it projects the z components as well.
So, when we see screenPosition.dimensions always comes out to 3 since we are storing other component with 0. Here I am preserving the count with original arguments and makes the test pass. Since it's not the actual fix and tends to be a workaround for the future release, I think this should work, Let me know if there's any objection @davepagurek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I'm understanding the flow here correctly:
- If you pass two numbers into
screenToWorld(e.g.screenToWorld(width/2, height/2)) or if you pass a single vector in that was created with two numbers (e.g.screenToWorld(createVector(1, 2))), we want to generate a z value - We can directly check the arguments length, but when the arg is a single vector, its values get padded to length 3, so we don't know directly how many arguments were manually provided
- We store a global
dimensionvalue corresponding to the number of args passed to the lastcreateVectorcall
That sounds like it works. I wonder if it might be a little more direct, instead of using global state, to edit createVector to instead set a secret internal property on the vector before returning it, like _origDimension, so then we could check if that exists on any vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I am using _origDimension now, if screenToWorld(width/2, height/2) we take the argument from there, and if we have not a number (probably a vector) we fetch the _origDimension, other logic are same? Also, now we are not using global state. Do you think it's correct for now?
|
Hi @perminder-17 and @davepagurek! Thanks so much for working on this. I'm really glad we're trying to figure out how to accommodate the 1.x Since I've been organizing the This implementation seems to revert Here are the key conflicts I've identified:
A Real-Time Example of This ComplexityThis PR's discussion of the To fix the bug this patch created, a new "secret internal" SummaryThis PR would significantly complicate the entire vector class, for all 2.x users, in order to accommodate a legacy edge case in a single function. Fundamentally, this patch replaces the simpler, more robust 2.x model with the legacy 1.x model. This breaks or blocks improvements that have already been added or are actively being worked on, based on extensive discussions (including the concept of true 1D and 2D vectors, as discussed in #8118). Proposal: Move this back to #8156 for alternativesSince this patch appears to have a much larger blast radius than intended, I propose we continue to discuss viable options in #8156, before we merge. I think we need to find a way to patch the My sense is that the simplest, lowest-impact solution may be to include a short note in the compatibility README, instructing users how to fix their sketches by supplying explicit Thanks again for all your work on this! |
I think we aren't trying to reach a permanent solution in this, but just unblock sketches that are currently broken. So it's very likely that we undo whatever we do in this PR when making larger changes to broadcasting. That said, do you think it would be reasonable to do a similar kind of padding, but only when operating on another vector for now? So like, temporarily implementing a version of broadcasting where we pad with 0s, but only when you add two vectors. So if you did: const a = createVector(1, 2) // Dimension is 2
const b = createVector(3, 4, 5) // Dimension is 3
a.add(b) // Maybe this logs an FES message saying you've multiplied two different-sized vectors together
// A's dimension is now 3, its data is [4, 6, 5]This would make |
|
Hi @davepagurek! Thank you so much for your quick reply. I love seeing all these ideas being put forth, and your new idea is really interesting. We're totally in agreement about wanting to fix the current user-facing bugs as quickly as possible. With that in mind, I have a proposal for a different quick fix, a few concerns about the "stopgap" approach, and what I see as a path to immediately unlock quick, permanent fixes. A truly quick fixIf our goal is to unblock users today with the lowest possible effort and zero technical debt, I think the fastest path is to add a note to the compatibility README and the 2.0 beta documentation. Something like this: "In 1.x, This is a 10-minute, no-code change that immediately solves the user's "why is my sketch broken?" problem. Crucially, we can reinforce this with a dedicated Friendly Error System message. That way, the README and reference pages provide advance notice, and users who are still bitten by the bug will find an immediate, simple solution when and where it happens. Risks of a custom "stopgap"My main concern with the stopgap you proposed (padding with 0s for This rule is precisely the same in every major library for math, machine learning, and when applicable, creative-coding. For example, openFrameworks also follows the same rule. Violating the rule means we'd have to code in warning messages that teach users special behavior that they'll have to unlearn—the permanent solution explicitly disallows this exact behavior. This feels like the "whack-a-mole" problem: we'd be solving the An opportunity for quick, permanent fixesThis brings me to what I think is the most exciting and high-leverage opportunity. The permanent solutions for this bug (and many others) are already clearly defined, the key policies like broadcasting have strong community support, and the issues have volunteers ready and waiting to implement them. The only thing preventing us from fixing these bugs quickly and permanently is that we're blocked on finalizing a few key policy decisions. I believe the most effective use of our time is to unblock the community. If the maintainer team can reach a final decision on these three key issues, it would unleash all of that volunteer energy:
Once these policies are approved, the community can immediately start implementing permanent fixes for these What do you think of this approach? It seems like a way to get the best of both worlds: a zero-debt immediate fix (the README) and a clear plan to rapidly unlock the permanent fixes. |
|
Hi @GregStanton thank you for your thoughtful review! After going through this with @davepagurek , here are my takeaways and recommendation for this PR:
While considering this I also referred to Dan Shiffman's comment and from what I could tell, although the code examples in that comment won't work with this patch, the teaching approach does use constructors with explicit arguments, so this supports keeping the scope of this fix separate from broadcasting fix. Thanks all for your careful work on this! |
Resolves #8156
Changes:
Screenshots of the change:
PR Checklist
npm run lintpasses