GNOME Bugzilla – Bug 682782
Remove ClutterKnot
Last modified: 2013-05-06 17:18:37 UTC
the only API using ClutterKnot is ClutterPath; we have ClutterPoint, now, so: - ClutterPathNode should be moved to ClutterPoint - ClutterKnot should be removed from the base types
also, we should drop the bezier computations done in fixed point math, and just use floating point: this allows us to increase range and precision, and avoid crappy results with long or scaled up paths.
Created attachment 240979 [details] [review] Patch 0001 I broke down the removal of ClutterKnot in three parts. First: Migrating ClutterBezier to float. Second: Migrating ClutterPathNone and ClutterPath to use ClutterPoint and third: Remove ClutterKnot
Created attachment 241067 [details] [review] Second patch. Removing ClutterKnot from ClutterPathNode This is the second stage. Removed ClutterKnot from almost every possible place. Replaced ClutterKnot for ClutterPoint inside CluterPathNode. This is the biggest change. This caused all ClutterPath API migrated to using gfloat instead of gint.
Created attachment 241150 [details] [review] Final patch. Removed ClutterKnot Final patch. Removed ClutterKnot
Review of attachment 240979 [details] [review]: it would probably be best if we added a bunch of unit tests to ClutterPath that verified that the curve behaves as intended, to catch regressions. ::: clutter/clutter-bezier.c @@ +81,3 @@ +} + +/** private functions should not use /** otherwise gtk-doc will pick them up and complain. you can either use a simple /* or do something like /*< private >. @@ +94,3 @@ } +/** same as above. @@ +107,3 @@ } +/** same as above. @@ +157,3 @@ } +/** same as above. @@ +199,3 @@ + b->cy = 3 * y_2; + b->dx = x_3; + b->dy = y_3; the initialization of the coefficients is different; the original code used the normalized values of the control points. have you tested ClutterPath with this commit? @@ +222,3 @@ } +/** the gtk-doc preamble should go away. @@ +257,3 @@ } +/** same as above.
Review of attachment 241067 [details] [review]: looks okay. ::: clutter/clutter-path.c @@ +344,3 @@ } +/** this is a static function, and should not have a gtk-doc preamble; you can use /* or /*< private > instead. ::: clutter/clutter-script-parser.c @@ +326,2 @@ gboolean _clutter_script_parse_knot (ClutterScript *script, we already have _clutter_script_parse_point(); this code should just go away.
Review of attachment 241150 [details] [review]: ::: doc/cookbook/examples/events-pointer-motion-scribbler.c @@ +17,3 @@ gpointer data) { + ClutterPoint knot; the events-pointer-motion-scribbler.c chunk should probably go into its own commit, before the commit the removes ClutterKnot from the code base.
Created attachment 241458 [details] [review] Migrating ClutterBezier (In reply to comment #5) > Review of attachment 240979 [details] [review]: > > it would probably be best if we added a bunch of unit tests to ClutterPath that > verified that the curve behaves as intended, to catch regressions. Yes, indeed. > @@ +199,3 @@ > + b->cy = 3 * y_2; > + b->dx = x_3; > + b->dy = y_3; > > the initialization of the coefficients is different; the original code used the > normalized values of the control points. have you tested ClutterPath with this > commit? Yeap, I did. The thing is the original code used the other form of expressing the cubic bezier as form of two quadratic bezier. I'm using the polynomial form of it
I fixed the patches. There for of them now.
Created attachment 241459 [details] [review] Migrating ClutterPathNode Fixed doc strings
Created attachment 241460 [details] [review] Updated code example
Created attachment 241461 [details] [review] Removing ClutterKnot Added fix to pass 'make check'
Created attachment 243091 [details] [review] Remove ClutterKnot from clutter2-sections doc file
Review of attachment 241458 [details] [review]: let's go with this.
Review of attachment 241459 [details] [review]: okay.
Review of attachment 241460 [details] [review]: okay.
Review of attachment 241461 [details] [review]: mmh, a bunch of whitespace fixes ended up in the patch. you may want to submit it as a separate patch.
Review of attachment 243091 [details] [review]: okay.
Created attachment 243186 [details] [review] Remove ClutterKnot withuot the whitespace fixes
Created attachment 243199 [details] [review] Whitespace and indentation fixes
Review of attachment 243186 [details] [review]: looks good.
Review of attachment 243199 [details] [review]: not a huge fan of this kind of commits: they tend to mess up git blame.
everything's been pushed, so let's close this.