Closed luigi-rosso closed 5 years ago
Wow! This is great! I'm glad you're finding the library useful. π
It's very cool to get a PR (complete with documentation) instead of just a feature request. Thanks for that! It might take me a few days before I have a chance to sit down and try this out, but I'm excited to see how it works.
If it's okay with you I'll probably merge this as it is, then do a quick pass on top of it myself to keep the style consistent and add a few automated tests. I might also add an option to pass in a cached version of the look-up table so that it doesn't have to be recomputed if you're calling project
multiple times on the same curve. π
Thanks again for the contribution! This totally made my day. βοΈ
Of course, you're most welcome! I was going to add a small test for it too, I can do that if you want to just keep this open for a few days.
I was wondering if it'd make sense to cache the look up table internally. You'd have to invalidate it when the control points change, but you're right it'd be so much faster for subsequent projections. What's nice about that is that the implementor doesn't have to worry about rebuilding the look up. Externally works too, however, just puts more work on the implementor. I use this to detect the proximity of the cursor to the curve (and get a point of reference for performing a split operation), some form of caching would definitely help.
No worries about the tests--I'm happy to take care of them! Testing in Dart is about as painless as you can get, and adding them myself will help me feel more confident in the change. β
I think that's a great point about caching the look-up table internally. π This is actually something I wrestled with a lot while designing the library's API, and I can certainly see the pros and cons of each approach. Bezier.js does cache look-up tables (along with many other properties) internally, and I agree that it's more convenient for the user in most cases. I chose to depart from the example of Bezier.js here for a few reasons, but I'm trying to keep an open mind about it, and I haven't totally ruled out switching over to internal caching for future releases. For what it's worth, here's how I've been looking at it. π
I'm assuming that most devs will never hit a performance bottleneck. I could be wrong about that, but there isn't really a good way to know for sure until the library has been in the wild for a while. π I figure that it's better to optimize for other factors like reliability, maintainability, and flexibility first. I think it's still important to provide performance optimizations for power users--after all, as a game developer, I'm one of them. But I don't want to hinder flexibility or maintainability in the process, since most users aren't even worried about speed.
It's true that internal caching usually does what you want, but there are some situations where it can be more cumbersome than helpful. For example, imagine you're writing a game which needs to do calculations on thousands and thousands of curves in each level. To speed up those calculations, you want to cache all of their look-up tables. Of course, it makes sense to calculate the look-up tables when the level is loading rather than during gameplay, so you don't get any performance hits. With internal caching, the only way to accomplish this would be to call positionLookUpTable
on every curve and then discard the result. That's not the worst thing in the world, but it's pretty awkward. π
It's also a little more difficult from the perspective of a first-time library user to figure out that caching is even a possibility when it's not an explicit part of the library's interface. Sure, you can just describe the implicit caching behavior in the documentation, but ideally devs should be able to make sense of the library purely by working with it, without taking their eyes off the code. Hopefully my habits are pretty representative of most devs'... but typically I only read the docs when I'm stuck. π― π»
So far, I've been building the Bezier
class so that its only mutable properties are the points in the points
list. There's two main advantages here, and they're closely related. First, the library is a lot easier to test. The more internal state an object has, the more tests you have to write to be confident that every possible permutation of its state is covered. But the second advantage is maintainability. Each time I make a change to the library, there's a good chance I'm going to forget to update some part of the state. I'm of the philosophy that fewer moving parts means less likely to break. βοΈ
But the last consideration... and I promise I'm almost done π
... has to do with the way the vector math library was designed. Since I'm exposing the points
list, and each point is mutable, I can't see a good way to detect when a particular point has changed. For example, I'm not sure how to handle this type of situation:
final curve = new QuadraticBezier([
new Vector2(10.0, 10.0),
new Vector2(70.0, 95.0),
new Vector2(25.0, 20.0)
]);
// Cache the look-up table internally here.
final lookUpTable = curve.positionLookUpTable();
// Changing the coordinates of one of the points should invalidate the cache,
// but I can't figure out a reasonable way to detect that the coordinates have changed.
curve.points[1].x = 30.0;
// This result may be wrong now.
final result = curve.project(new Vector2(40.0, 35.0));
I could just say "you're never allowed to change the values in points
after a curve is created," but that would make Bezier
objects pretty hard to work with. π¦
Oookay, I'm so sorry about that ridiculously long response. π³ But it kind of helped me to outline my thought process here. I'm sure I'll come back to it later. And hopefully it will make some sense to others as well. πΈ
Anyway, I'm optimistic I'll be able to merge this and make a new release of the library tomorrow. βοΈ
Quick update: Yesterday I made my own version of the project
method, based on the work you started. π I think it's all ready, but I want to finish building a simple app with it first to make sure, so it's taking a little bit longer. β³π I think I'll close this PR and post my own (which includes this commit) this afternoon. I'd like @IkeBart to review it too to make sure I didn't mess up anything. But if all goes well, prepare for launch of v1.1.0
! π π
Thanks for doing this, I did not mean to give you extra work. Awesome that you took it upon yourself to implement it so thoroughly. Thank you!
Regarding your question, I was thinking about that as I was typing my last message...maybe returning a copy of the points when getting them is clearest. Manipulating the X/Y values of a retrieved point won't change the internal state. You could supply a helper method to reset/change them individually, but I think that's just unnecessary bloat. Most cases you'll need to redo all the work anyway, so newing another object is pretty efficient. Which is another point in favor of internal caching :) although I agree with your comment regarding power users, so again, I'm fine with whichever choice you make, as long as there's a caching option.
Let me know if I can help further.
Oh, it's no problem! @IkeBart and I have been trying to build the best library on the internet for BΓ©zier math, and I feel like we're a little closer to that goal today. π I'm also confident that plenty of others will find the feature useful down the road. Best of luck with the rest of your experiment! π¨βπ¬ π
Thanks for the awesome library. I took a peek yesterday with a little experiment I'm doing and noticed I needed the projection op equivalent from bezierjs.