mass conversion to Geom::Point
Hi all,
I just started mass converting to Geom::Point, but then it struck me that this will conflict with many changes other people are creating.
So I think it is best not to mass convert. It is the same with whitespace things.
Let me know what you all think.
NR::Point contains cast methods, so Geom::Point and NR::Point are interchangeable in 90% of the cases. It would help a lot if you'd just change NR::Point to Geom::Point in a line you are editing.
Thanks! Johan
J.B.C.Engelen@...1578... wrote:
I just started mass converting to Geom::Point, but then it struck me that this will conflict with many changes other people are creating.
That was my first thought also when I read the subject of your mail. Maybe this should wait until just after the next release?
Regards,
Diederik
Diederik van Lierop wrote:
J.B.C.Engelen@...1578... wrote:
I just started mass converting to Geom::Point, but then it struck me that this will conflict with many changes other people are creating.
That was my first thought also when I read the subject of your mail. Maybe this should wait until just after the next release?
Perhaps my opinion isn't relevant right now because I'm not actively working on anything but... The focus of this release cycle is nominally on refactoring. I we're postponing refactoring something is wrong. With a mass conversion there will be pain once for everyone. Without a mass conversion NR::Point will live on forever as it is continually added to the code base again and again. I say convert things and then remove NR::Point along with the temptation to use it. Alternatively give people a week to do a SVN diff and update what they are working on themselves and after the week convert everything else. Feel free to disagree. :-)
Aaron Spike
On Sun, Aug 03, 2008 at 06:15:51AM -0500, Aaron Spike wrote:
Diederik van Lierop wrote:
J.B.C.Engelen@...1578... wrote:
I just started mass converting to Geom::Point, but then it struck me that this will conflict with many changes other people are creating.
That was my first thought also when I read the subject of your mail. Maybe this should wait until just after the next release?
Perhaps my opinion isn't relevant right now because I'm not actively working on anything but... The focus of this release cycle is nominally on refactoring. I we're postponing refactoring something is wrong. With a mass conversion there will be pain once for everyone. Without a mass conversion NR::Point will live on forever as it is continually added to the code base again and again. I say convert things and then remove NR::Point along with the temptation to use it. Alternatively give people a week to do a SVN diff and update what they are working on themselves and after the week convert everything else. Feel free to disagree. :-)
Agreed with all the above. The purpose of this release is refactoring, so refactor away.
Bryce
-----Original Message----- From: Diederik van Lierop [mailto:mail@...1689...] Sent: zondag 3 augustus 2008 8:44 To: Engelen, J.B.C. (Johan) Cc: inkscape-devel@lists.sourceforge.net Subject: Re: [Inkscape-devel] mass conversion to Geom::Point
J.B.C.Engelen@...1578... wrote:
I just started mass converting to Geom::Point, but then it
struck me
that this will conflict with many changes other people are creating.
That was my first thought also when I read the subject of your mail. Maybe this should wait until just after the next release?
I'll wait on your snapping change, as that is a big change I think. After that, I'll start mass converting.
-johan
J.B.C.Engelen@...1578... schrieb:
I'll wait on your snapping change, as that is a big change I think. After that, I'll start mass converting.
I'd appreciate if, as Aaron suggested, you could name a date when you start working and/or are going to commit the changes. Would it be okay to wait for a couple of days (but announce it in advance) so that people can commit stuff they are working on? (I have a few things waiting myself, which I'd be glad to commit first).
Thanks, Max
-----Original Message----- From: inkscape-devel-bounces@lists.sourceforge.net [mailto:inkscape-devel-bounces@lists.sourceforge.net] On Behalf Of Maximilian Albert Sent: maandag 4 augustus 2008 14:52 To: Engelen, J.B.C. (Johan) Cc: inkscape Subject: Re: [Inkscape-devel] mass conversion to Geom::Point
J.B.C.Engelen@...1578... schrieb:
I'll wait on your snapping change, as that is a big change I think. After that, I'll start mass converting.
I'd appreciate if, as Aaron suggested, you could name a date when you start working and/or are going to commit the changes. Would it be okay to wait for a couple of days (but announce it in advance) so that people can commit stuff they are working on? (I have a few things waiting myself, which I'd be glad to commit first).
I am going to start this evening. One directory each commit. Which dirs shouldn't I touch? (for you Max, I guess I shouldn't touch root, lpe and display?)
J.B.C.Engelen@...1578... schrieb:
I am going to start this evening. One directory each commit. Which dirs shouldn't I touch? (for you Max, I guess I shouldn't touch root, lpe and display?)
Sorry for the long delay in getting back to you (I had problems with my internet connection). After double checking, from my point of view you proceed safely even with the above-mentioned ones. If any conflicts will arise at all I can hopefully resolve them easily. Don't know about other people, though. :)
Cheers, Max
I could really use some help with this. Please help converting NR:: to Geom:: for the following types:
NR::Point => Geom::Point NR::Matrix => Geom::Matrix NR::scale => Geom::Scale NR::translate => Geom::Translate NR::rotate => Geom::Rotate NR::Rect => Geom::Rect NR::Coord => Geom::Coord NR::Dim2 => Geom::Dim2 and perhaps some other types as well. (*not* NR::IRect, NR::Filter*)
I use the attached script. (replaces 'NR::' with 'Geom::') I tweak it a bit to then convert some things back to NR. (like Geom::Filter back to NR::Filter)
The conversion work is pretty simple (read: dumb) work. Run the script, then try to compile and fix all errors that pop up. Usually fixing an error involves adding the right include files, or adding to_2geom or from_2geom. Note the capital for scale, translate and rotate (the Geom::Scale and friends are found in <2geom/transforms.h>
Thanks a lot, I am getting burned out on this one :(
Johan
Heh, thanks.
-----Original Message----- From: Diederik van Lierop [mailto:mail@...1689...] Sent: woensdag 6 augustus 2008 23:17 To: Engelen, J.B.C. (Johan) Cc: inkscape-devel@lists.sourceforge.net Subject: Re: [Inkscape-devel] mass conversion to Geom::Point
J.B.C.Engelen@...1578... wrote:
I use the attached script. (replaces 'NR::' with 'Geom::')
Johan, I don't see any attachment. You must be really getting tired ;-)
Diederik
-----Original Message----- From: Engelen, J.B.C. (Johan) Sent: woensdag 6 augustus 2008 22:10 To: inkscape-devel@lists.sourceforge.net Subject: RE: [Inkscape-devel] mass conversion to Geom::Point
I could really use some help with this. Please help converting NR:: to Geom:: for the following types:
NR::Point => Geom::Point NR::Matrix => Geom::Matrix NR::scale => Geom::Scale NR::translate => Geom::Translate NR::rotate => Geom::Rotate NR::Rect => Geom::Rect NR::Coord => Geom::Coord NR::Dim2 => Geom::Dim2 and perhaps some other types as well. (*not* NR::IRect, NR::Filter*)
I use the attached script. (replaces 'NR::' with 'Geom::') I tweak it a bit to then convert some things back to NR. (like Geom::Filter back to NR::Filter)
The conversion work is pretty simple (read: dumb) work. Run the script, then try to compile and fix all errors that pop up. Usually fixing an error involves adding the right include files, or adding to_2geom or from_2geom. Note the capital for scale, translate and rotate (the Geom::Scale and friends are found in <2geom/transforms.h>
Often it is not needed to add to_2geom/from_2geom, so please don't! The conversion goes automatically between NR::Point <=> Geom::Point and NR::Matrix <=> Geom::Matrix. Only ambiguities need a little more guideance: void some_function(NR::Point nr) Geom::Point geom; Geom::Point ambiguous = nr - geom;
Possible fix:
Geom::Point ambiguous = to_2geom(nr) - geom;
But, if the type of nr changes to Geom::Point, we have to edit this line again to remove the "to_2geom". So it is better to do:
Geom::Point ambiguous = (Geom::Point)nr - geom;
Thanks!
Johan
On Aug 2, 2008, at 9:03 PM, J.B.C.Engelen@...1578... wrote:
I just started mass converting to Geom::Point, but then it struck me that this will conflict with many changes other people are creating.
So I think it is best not to mass convert. It is the same with whitespace things.
Let me know what you all think.
One thing I would do would be to see if the mass change could be done piecemeal. That is, perhaps it can be done and checked in on a module- by-module basis? That might also allow for a bit of smoke-testing before each commit.
Oh, and just be sure to do "make check" and also use valgrind if you can. Those might show some problems that otherwise might take a while to notice and track down.
Same as when we did the C++-ification. Do it. Do it now and get it over with. We can fix the bugs. Like with my recent education into using Geom::Point, Matrix, blah rather than NR, it's quite painless, and sensible. Actually, Geom has a few "lessons learned" that make it a bit easier than NR.
Besides, 2Geom has the Nathan J Hurst seal of approval, which is good enough for me.
bob
On Aug 3, 2008, at 11:00 PM, Bob Jamison wrote:
Same as when we did the C++-ification. Do it. Do it now and get it over with. We can fix the bugs. Like with my recent education into using Geom::Point, Matrix, blah rather than NR, it's quite painless, and sensible. Actually, Geom has a few "lessons learned" that make it a bit easier than NR.
First compile "Boom"
/usr/bin/ld: Undefined symbols: BPath_from_2GeomPath(std::vector<Geom::Path, std::allocatorGeom::Path > const&) collect2: ld returned 1 exit status make[2]: *** [inkscape] Error 1
________________________________
From: inkscape-devel-bounces@lists.sourceforge.net [mailto:inkscape-devel-bounces@lists.sourceforge.net] On Behalf Of Jon A. Cruz Sent: maandag 4 augustus 2008 8:27 To: inkscape Subject: Re: [Inkscape-devel] mass conversion to Geom::Point On Aug 3, 2008, at 11:00 PM, Bob Jamison wrote:
Same as when we did the C++-ification. Do it. Do it now and
get it over with. We can fix the bugs. Like with my recent
education into using Geom::Point, Matrix, blah rather than NR,
it's quite painless, and sensible. Actually, Geom has a few
"lessons learned" that make it a bit easier than NR.
First compile "Boom" /usr/bin/ld: Undefined symbols: BPath_from_2GeomPath(std::vector<Geom::Path, std::allocatorGeom::Path > const&) collect2: ld returned 1 exit status make[2]: *** [inkscape] Error 1
I actually didn't do anything yet.
I'll probably do the change per subdirectory.
participants (7)
-
unknown＠example.com -
Aaron Spike -
Bob Jamison -
Bryce Harrington -
Diederik van Lierop -
Jon A. Cruz -
Maximilian Albert