Re: [Kicad-developers] VECTOR2I and VECTOR2D
Hi Simon, Thanks for the update. I will continue with my work as planned. I think the only conflict will be in the include/convert_to_biu.h which will only be the schematic units section. Cheers, Wayne On 6/25/2019 3:16 PM, Simon Richter wrote: > Hi Wayne, > > On Tue, Jun 25, 2019 at 12:36:36PM -0400, Wayne Stambaugh wrote: > >> I guess I should comment on this seeing that I am the project leader. I >> am fine with refactoring as long as it's an improvement over existing >> code. > > The main improvement is going to be that we can dump the preprocessor magic > for the internal units, which then allows us to share binary code between > different kifaces, so we can turn common into a shared library. > > With that: > > - binary size is halved > - qa_common tests can be merged > - tests can be linked against a shared library (way faster) > - the python module can be linked against a shared library > - we no longer have different functions with the same name (great for >debugging) > > So yes, I think that'd be worth it. Getting nice unit names and some type > safety is just the icing on the cake (but necessary for refactoring, so the > compiler can tell us where the next change needs to be made). > >> What changes are you planning for units and when do you expect to have >> this done? I just branched myself to port the schematic internal units >> to 100nm in preparation for the new file formats so I'm sure there is >> going to be conflicts. If you are almost ready to merge, I can wait but >> I'm ready to get started on this so I would rather not wait too long. > > My approach for the length units can be merged incrementally, so there is > an intermediate stage where it will convert between "old" and "new" > internal units at the boundaries (by multiplying or dividing with the > appropriate factor according to the -D flag from the command line) and > issue a warning "refactor here". > > So I can effectively rebase my branch on top of whatever changes you have, > the compatibility code will keep it working. Merging would happen in > multiple stages anyway, with the compatibility code remaining in place > until everyone's development branches are merged or rebased. > > That's why I listed > >>> Replacing points will be too big to do in a single commit, so: >>> >>> - create strong types for lengths and points [needs rework] >>> - literal handling with unit suffix [mostly done] >>> - compatibility code for automatic conversion to existing formats [mostly >>>done] >>> - piecewise replacement of existing coordinate code >>> - remove compatibility code >>> - remove old code >>> - remove -DEESCHEMA, -DPCBNEW etc. >>> - merge pcbcommon into common > > Each of these steps live in separate commits, and every commit takes us > from a working state to another working state. > >Simon > > ___ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 25/06/2019 21:16, Simon Richter wrote: > Hi Wayne, > > On Tue, Jun 25, 2019 at 12:36:36PM -0400, Wayne Stambaugh wrote: > >> I guess I should comment on this seeing that I am the project leader. I >> am fine with refactoring as long as it's an improvement over existing >> code. > > The main improvement is going to be that we can dump the preprocessor magic > for the internal units, which then allows us to share binary code between > different kifaces, so we can turn common into a shared library. > Hi Simon, I guess you're going to add custom vector/point/dimension types that contain units information? I would like to leave basic data structures (e.g. VECTOR2I) unit-less. Tom ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
Hi Wayne, On Tue, Jun 25, 2019 at 12:36:36PM -0400, Wayne Stambaugh wrote: > I guess I should comment on this seeing that I am the project leader. I > am fine with refactoring as long as it's an improvement over existing > code. The main improvement is going to be that we can dump the preprocessor magic for the internal units, which then allows us to share binary code between different kifaces, so we can turn common into a shared library. With that: - binary size is halved - qa_common tests can be merged - tests can be linked against a shared library (way faster) - the python module can be linked against a shared library - we no longer have different functions with the same name (great for debugging) So yes, I think that'd be worth it. Getting nice unit names and some type safety is just the icing on the cake (but necessary for refactoring, so the compiler can tell us where the next change needs to be made). > What changes are you planning for units and when do you expect to have > this done? I just branched myself to port the schematic internal units > to 100nm in preparation for the new file formats so I'm sure there is > going to be conflicts. If you are almost ready to merge, I can wait but > I'm ready to get started on this so I would rather not wait too long. My approach for the length units can be merged incrementally, so there is an intermediate stage where it will convert between "old" and "new" internal units at the boundaries (by multiplying or dividing with the appropriate factor according to the -D flag from the command line) and issue a warning "refactor here". So I can effectively rebase my branch on top of whatever changes you have, the compatibility code will keep it working. Merging would happen in multiple stages anyway, with the compatibility code remaining in place until everyone's development branches are merged or rebased. That's why I listed > > Replacing points will be too big to do in a single commit, so: > > > > - create strong types for lengths and points [needs rework] > > - literal handling with unit suffix [mostly done] > > - compatibility code for automatic conversion to existing formats [mostly > >done] > > - piecewise replacement of existing coordinate code > > - remove compatibility code > > - remove old code > > - remove -DEESCHEMA, -DPCBNEW etc. > > - merge pcbcommon into common Each of these steps live in separate commits, and every commit takes us from a working state to another working state. Simon ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
I guess I should comment on this seeing that I am the project leader. I am fine with refactoring as long as it's an improvement over existing code. In this case, I would say that some refactoring is in order although I would proceed cautiously with the code in question. OOP does not necessary translate to more maintainable and/or understandable code. Please don't confuse programming paradigms for religion. I have been at this for 30 years and have heard the promises (sales pitches?) of how every new programming paradigm was going to save us from ourselves. I'm still waiting ;) On 6/23/19 11:03 AM, Simon Richter wrote: > Hi, > > On Fri, Jun 21, 2019 at 10:54:04PM -0400, Reece R. Pollack wrote: > >> Just for discussion, let's assume the replacement for wxPoint is >> named KiPoint. The result of subtracting two KiPoint objects would >> be another object called KiDelta. Adding two KiPoint objects should >> be undefined, and result in a compile-time error, as this is a >> nonsense operation. > > I have something like that cooking in my "units" branch, but I'm cleaning > up angles first. Simon, What changes are you planning for units and when do you expect to have this done? I just branched myself to port the schematic internal units to 100nm in preparation for the new file formats so I'm sure there is going to be conflicts. If you are almost ready to merge, I can wait but I'm ready to get started on this so I would rather not wait too long. Cheers, Wayne > > Plan: > > - create strong types for angles and orientations [done] > - literal handling with unit suffix [done] > - replace all instances of angles in the codebase [under way] > - drop code to support angles expressed as doubles, ints, ... > > Replacing points will be too big to do in a single commit, so: > > - create strong types for lengths and points [needs rework] > - literal handling with unit suffix [mostly done] > - compatibility code for automatic conversion to existing formats [mostly >done] > - piecewise replacement of existing coordinate code > - remove compatibility code > - remove old code > - remove -DEESCHEMA, -DPCBNEW etc. > - merge pcbcommon into common > > The way I went with my approach was that there is a length class that > expresses a 1D length in either nanometers. A separate class does the same > for pixels. > > 2D coordinates have a template parameter for the underlying 1D type, and > for the origin point they are relative to: > > - no origin > - (0, 0) > - current reference (set with the space bar) > - auxiliary axis (set with the aux axis tool) > > Adding and subtracting then makes sure origin points are consistent, and > multiplying with the current zoom level converts between world and screen > coordinates. > > The initial implementation went a bit wrong because the angle handling > needs to be done first -- the transition requires adding a new coordinate > type parallel to the existing ones, which also requires a full set of > Rotate* functions (degrees as double, tenth-degrees as double, > tenth-degrees as signed int, radians as double), so reducing all of these > to one makes this manageable. > >Simon > > ___ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
Hi, On Fri, Jun 21, 2019 at 10:54:04PM -0400, Reece R. Pollack wrote: > Just for discussion, let's assume the replacement for wxPoint is > named KiPoint. The result of subtracting two KiPoint objects would > be another object called KiDelta. Adding two KiPoint objects should > be undefined, and result in a compile-time error, as this is a > nonsense operation. I have something like that cooking in my "units" branch, but I'm cleaning up angles first. Plan: - create strong types for angles and orientations [done] - literal handling with unit suffix [done] - replace all instances of angles in the codebase [under way] - drop code to support angles expressed as doubles, ints, ... Replacing points will be too big to do in a single commit, so: - create strong types for lengths and points [needs rework] - literal handling with unit suffix [mostly done] - compatibility code for automatic conversion to existing formats [mostly done] - piecewise replacement of existing coordinate code - remove compatibility code - remove old code - remove -DEESCHEMA, -DPCBNEW etc. - merge pcbcommon into common The way I went with my approach was that there is a length class that expresses a 1D length in either nanometers. A separate class does the same for pixels. 2D coordinates have a template parameter for the underlying 1D type, and for the origin point they are relative to: - no origin - (0, 0) - current reference (set with the space bar) - auxiliary axis (set with the aux axis tool) Adding and subtracting then makes sure origin points are consistent, and multiplying with the current zoom level converts between world and screen coordinates. The initial implementation went a bit wrong because the angle handling needs to be done first -- the transition requires adding a new coordinate type parallel to the existing ones, which also requires a full set of Rotate* functions (degrees as double, tenth-degrees as double, tenth-degrees as signed int, radians as double), so reducing all of these to one makes this manageable. Simon ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
Hi Tom, On 23/06/2019 4:10 AM, Tomasz Wlostowski wrote: On 22/06/2019 17:41, Reece R. Pollack wrote: While it is true that you can add two point coordinates and multiply by scalar 0.5 to get the midpoint, this is not true in the general case for arbitrary scalar multipliers. However, calculating the vector distance between two points, multiplying the vector by a scalar, then adding the resulting vector distance to the first point /does/ work in the general case. This is exactly the sort of bug that can be avoided by not allowing arbitrary operations on random vectors. I've never experienced a bug caused by mixing points/vectors together, at least in the math code. For passing coordinates/measurements from/to the GUI it might make sense to create custom types. Moreover, most if not all geometry libraries I've known used the same class for points and vectors. Single class for both is IMHO Occam's razor approach. As Seth already remarked, I would like to hear a solid argument for splitting point/vector classes, otherwise it looks like bikeshedding to me. While it's cool that you have such a strong grasp of geometry operations and how they map to linear algebra, not everyone is at your level. Potential contributors to Kicad will have all sorts of backgrounds and be in the process of learning about geometry and or linear algebra. The math vector + geometric point VECTOR2 class technically works, but creates friction because to use it confidently one needs to read the class code and know a bit about how geometry operations are mapped to vector operations. Splitting the classes into Vector, Point, and Delta (Spatial Vector), and restricting the behavior of each to match their math and geometry counterparts, will make it much easier for others to learn and come onboard. Meanwhile, since you don't use spatial vectors erroneously, you won't see any new compiler errors, but people who are still learning about geometry will get useful feedback. -Hauptmech Tom PS1. I'm surprised no one yet noticed the VECTOR2<> class has public x/y members, which is an unforgivable violation of the tenets of classical OO design :-) PS2. There are some more serious OOD violations in KiCad codebase. Would anybody here volunteer to refactor the diamond in EDA_TEXT derivatives, DRAWSEGMENT/EDGE_MODULE classes or global variables in eeschema? ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
A specification for substantial changes like this should document clearly the hierarchy of inheritance, each class's functions and expected returns. There are some very good references out there on how to write specs [1][2][3]. This is not to dismiss Reece's suggestion. I think it is a good one and potentially quite beneficial long-term. I also think Reece is approaching this the right way by writing a short e-mail to the list first before pouring effort into the spec. I'm not quite certain whether Reece is suggesting that he'd like to make this change or whether it is being proposed as a wishlist item for future work. If it is the former, it needs a spec. If it is the latter, we can do that when it is on someone's to-do list. Best- Seth [1] https://www.joelonsoftware.com/2000/10/02/painless-functional-specifications-part-1-why-bother/ [2] https://books.google.com/books/about/Specification_by_Example.html?id=5F5PYgEACAAJ [3] https://codeburst.io/on-writing-tech-specs-6404c9791159 On 2019-06-22 21:37, hauptmech wrote: Isn't Reece's original post the spec? In summary, the name VECTOR is confusing and the type structure could be expanded on to offer more compile time checks. He proposes a KiPoint and KiDelta, and describes their behavior clearly enough for anyone that does geometry calculations. What's missing from the spec? On 23/06/2019 2:52 AM, Seth Hillbrand wrote: On 2019-06-21 22:54, Reece R. Pollack wrote: Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way. Hi Reece- Codebase cleaning like you suggest can go a long way toward improving sustainability and readability. But done the wrong way, it will have the opposite effect as we fight with return types that aren't fully matched. Before we decide to do this type of deep plumbing on the KiCad innards, I'd love to see the proposed specification for what's being implemented. Otherwise, this'll be a bike shedding discussion. -S ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
Isn't Reece's original post the spec? In summary, the name VECTOR is confusing and the type structure could be expanded on to offer more compile time checks. He proposes a KiPoint and KiDelta, and describes their behavior clearly enough for anyone that does geometry calculations. What's missing from the spec? On 23/06/2019 2:52 AM, Seth Hillbrand wrote: On 2019-06-21 22:54, Reece R. Pollack wrote: Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way. Hi Reece- Codebase cleaning like you suggest can go a long way toward improving sustainability and readability. But done the wrong way, it will have the opposite effect as we fight with return types that aren't fully matched. Before we decide to do this type of deep plumbing on the KiCad innards, I'd love to see the proposed specification for what's being implemented. Otherwise, this'll be a bike shedding discussion. -S ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 22/06/2019 17:41, Reece R. Pollack wrote: > > While it is true that you can add two point coordinates and multiply by > scalar 0.5 to get the midpoint, this is not true in the general case for > arbitrary scalar multipliers. However, calculating the vector distance > between two points, multiplying the vector by a scalar, then adding the > resulting vector distance to the first point /does/ work in the general > case. > > This is exactly the sort of bug that can be avoided by not allowing > arbitrary operations on random vectors. I've never experienced a bug caused by mixing points/vectors together, at least in the math code. For passing coordinates/measurements from/to the GUI it might make sense to create custom types. Moreover, most if not all geometry libraries I've known used the same class for points and vectors. Single class for both is IMHO Occam's razor approach. As Seth already remarked, I would like to hear a solid argument for splitting point/vector classes, otherwise it looks like bikeshedding to me. Tom PS1. I'm surprised no one yet noticed the VECTOR2<> class has public x/y members, which is an unforgivable violation of the tenets of classical OO design :-) PS2. There are some more serious OOD violations in KiCad codebase. Would anybody here volunteer to refactor the diamond in EDA_TEXT derivatives, DRAWSEGMENT/EDGE_MODULE classes or global variables in eeschema? ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 22/06/2019 16:32, hauptmech wrote: > After reading through vector2d.h and matrix3x3.h, I agree with Reece > more or less. There is ambiguity in the word vector, between math > vectors, spatial vectors, and c++ vectors. Context implies that VECTOR2 > refers math vectors, but then MATRIX3x3 * VECTOR2 is allowed which > violates expectations. POINT2 and SE2 or HOMOGENEOUS2 would set > expectations better. Actually, the name MATRIX3x3 name is a bit misleading - it actually represents a 2D affine transform. Anybody willing to send a patch renaming it to XFORM2D or something more descriptive? :) Tom ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 6/22/19 10:52 AM, Seth Hillbrand wrote: On 2019-06-21 22:54, Reece R. Pollack wrote: Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way. Hi Reece- Codebase cleaning like you suggest can go a long way toward improving sustainability and readability. But done the wrong way, it will have the opposite effect as we fight with return types that aren't fully matched. Before we decide to do this type of deep plumbing on the KiCad innards, I'd love to see the proposed specification for what's being implemented. Otherwise, this'll be a bike shedding discussion. I've been in far too many "code reviews" where the focus became the naming of variables and not on the algorithm being implemented. So I agree with your comment in principle. That said, the lack of type specificity already makes portions of our code hard to maintain. The DIALOG_GRAPHIC_ITEM_PROPERTIES class already reuses the m_endX UNIT_BINDER to represent both the X-axis endpoint of an object and the radius of a circle. This requires special handling when performing display origin transforms because the X-axis endpoint requires origin transformation while the circle radius does not. This illustrates the risk of treating everything as "just a pair of numbers" because it's "easy". I'm not suggesting rushing into a major change. But replacing a descriptive object like wxPoint with a non-descriptive object like VECTOR2I isn't an improvement. Especially when its implementation is somewhat non-intuitive, as Hauptmech points out. -Reece ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 6/22/19 3:09 AM, Greg Smith wrote: Adding two points and dividing by two results in a point that is minimally equidistant from both points (I.e. The midpoint of the line formed by the points). While it is true that you can add two point coordinates and multiply by scalar 0.5 to get the midpoint, this is not true in the general case for arbitrary scalar multipliers. However, calculating the vector distance between two points, multiplying the vector by a scalar, then adding the resulting vector distance to the first point /does/ work in the general case. This is exactly the sort of bug that can be avoided by not allowing arbitrary operations on random vectors. If finding the midpoint between two points proves to be a common operation, and is found to be too costly to implement in the general fashion, then a special method for determining the midpoint can be provided that calculates it in the optimal fashion. ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 2019-06-21 22:54, Reece R. Pollack wrote: Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way. Hi Reece- Codebase cleaning like you suggest can go a long way toward improving sustainability and readability. But done the wrong way, it will have the opposite effect as we fight with return types that aren't fully matched. Before we decide to do this type of deep plumbing on the KiCad innards, I'd love to see the proposed specification for what's being implemented. Otherwise, this'll be a bike shedding discussion. -S ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
After reading through vector2d.h and matrix3x3.h, I agree with Reece more or less. There is ambiguity in the word vector, between math vectors, spatial vectors, and c++ vectors. Context implies that VECTOR2 refers math vectors, but then MATRIX3x3 * VECTOR2 is allowed which violates expectations. POINT2 and SE2 or HOMOGENEOUS2 would set expectations better. If you are working with homogeneous coordinates and want to operate at the math vector & matrix level of abstraction (often my preference) then using a VECTOR3 would be clearer than hacking a VECTOR2. -Hauptmech On 22/06/2019 9:28 PM, Tomasz Wlostowski wrote: On 22/06/2019 09:09, Greg Smith wrote: I think the biggest point I am making is that, mathematically, a point is identical to a vector from 0,0. Hi Greg & Reece, This is precisely the reason why we don't have separate point and vector classes. Tom ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
The evolution from untyped variables to weakly-typed variables to strongly-typed variables to OOP techniques has never been about what is easiest for the programmer nor fastest running. It's about producing correct, reliable, maintainable software. The argument that "it'll be too slow" is almost never true and then almost always easily addressed. On 6/22/19 3:09 AM, Greg Smith wrote: In addition, treating points as vectors make a variety of required transformations much easier. Wrapping the two numbers in an object that disallows common transformations, calculations, and combinations seems unnecessary and will result in significantly slower code. ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
On 22/06/2019 09:09, Greg Smith wrote: > > I think the biggest point I am making is that, mathematically, a point > is identical to a vector from 0,0. > Hi Greg & Reece, This is precisely the reason why we don't have separate point and vector classes. Tom ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] VECTOR2I and VECTOR2D
In graphics/CAD packages, a point can be considered a vector from point 0,0. This is an interpretation that makes sense to me. Adding two points and dividing by two results in a point that is minimally equidistant from both points (I.e. The midpoint of the line formed by the points). In addition, treating points as vectors make a variety of required transformations much easier. Wrapping the two numbers in an object that disallows common transformations, calculations, and combinations seems unnecessary and will result in significantly slower code. I think the biggest point I am making is that, mathematically, a point is identical to a vector from 0,0. Greg S. > On Jun 21, 2019, at 10:04 PM, Reece R. Pollack wrote: > > Whoops, hit "send" too soon. "Is a simple array" is obviously incorrect. My > point was the lack of descriptiveness is a problem. The absence of > differentiation between a location and a distance is a missed opportunity. > And the abuse of the type as a loosely-related pair of values should be > considered a bug. > >> On 6/21/19 10:54 PM, Reece R. Pollack wrote: >> A while back, Jeff emailed me a note suggesting that KiCad was trying to >> decouple from the wxWidgets type wxPoint except where needed in the UI code. >> He suggested that I should use VECTOR2I instead. >> >> I understand and support the desire to decouple from wxWidgets. However, >> looking at the implementation of vector2d.h I was surprised to discover that >> VECTOR2I is a simple array of int like I would find in an old Fortran >> program. It's not even a C structure, let alone a well-defined C++ object. >> >> One of the goals of object-oriented programming is to create descriptive >> objects. Programmers can tell what an object represents by looking at the >> name and implementation of the object. Well-crafted object help avoid >> programming errors that could result from performing undefined operations or >> using data from incompatible objects. In geometry, if we subtract the 2d >> location of one point from the 2d location of another point the result is a >> 2d vector describing the distance between these points. However, this vector >> is not a point. In OOP this vector should be a different object than a point. >> >> The wxWidgets objects wxPoint and wxRealPoint are descriptively named, >> though poorly implemented. Aside from where code abuses these, they clearly >> represent locations. Replacing these with non-OOB typedefs such as VECTOR2I >> removes the clarity that a variable is intended to represent a location. The >> name is not descriptive of a location. There is no sanity check to prevent >> performing a nonsensical operation like adding two locations. It tells the >> programmer nothing about whether a variable contains a location, a length, >> or just two loosely-related values. >> >> Rather than replacing wxPoint with VECTOR2I, let us consider how we can make >> our code more object-oriented rather than more Fortran-like. I believe we >> should replace wxPoint and wxRealPoint with KiCad-specific objects. These >> objects should be named such that they clearly indicate that they of >> represent locations rather than a simple array of integer values. They >> should be implemented such that nonsensical operations are prohibited and >> sanity checks put in place where possible. For example, attempting to add >> two locations should result in a compile-time error. Additional objects >> should be defined to represent the delta between two locations, and the >> operations of adding a delta to a location should result in a location. >> >> Just for discussion, let's assume the replacement for wxPoint is named >> KiPoint. The result of subtracting two KiPoint objects would be another >> object called KiDelta. Adding two KiPoint objects should be undefined, and >> result in a compile-time error, as this is a nonsense operation. Adding a >> KiPoint and a KiDelta should return a KiPoint. We should never abuse a >> KiPoint to represent anything else, such as using the X value as the radius >> of a circle as is currently done. I suspect most of the places where this >> would be much more than a mechanical substitution is a place where the code >> abuses the type or is a latent bug. >> >> Doing this now, before we go too far down the path of replacing wxWidgets >> types with non-OOB arrays would enhance readability and make the code more >> robust. Using VECTOR2I is going the wrong way. >> >> -Reece >> >> >> ___ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : kicad-developers@lists.launchpad.net >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp > > ___ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe :
Re: [Kicad-developers] VECTOR2I and VECTOR2D
Whoops, hit "send" too soon. "Is a simple array" is obviously incorrect. My point was the lack of descriptiveness is a problem. The absence of differentiation between a location and a distance is a missed opportunity. And the abuse of the type as a loosely-related pair of values should be considered a bug. On 6/21/19 10:54 PM, Reece R. Pollack wrote: A while back, Jeff emailed me a note suggesting that KiCad was trying to decouple from the wxWidgets type wxPoint except where needed in the UI code. He suggested that I should use VECTOR2I instead. I understand and support the desire to decouple from wxWidgets. However, looking at the implementation of vector2d.h I was surprised to discover that VECTOR2I is a simple array of /int/ like I would find in an old Fortran program. It's not even a C structure, let alone a well-defined C++ object. One of the goals of object-oriented programming is to create descriptive objects. Programmers can tell what an object represents by looking at the name and implementation of the object. Well-crafted object help avoid programming errors that could result from performing undefined operations or using data from incompatible objects. In geometry, if we subtract the 2d location of one point from the 2d location of another point the result is a 2d vector describing the distance between these points. However, this vector is not a point. In OOP this vector should be a different object than a point. The wxWidgets objects wxPoint and wxRealPoint are descriptively named, though poorly implemented. Aside from where code abuses these, they clearly represent locations. Replacing these with non-OOB typedefs such as VECTOR2I removes the clarity that a variable is intended to represent a location. The name is not descriptive of a location. There is no sanity check to prevent performing a nonsensical operation like adding two locations. It tells the programmer nothing about whether a variable contains a location, a length, or just two loosely-related values. Rather than replacing wxPoint with VECTOR2I, let us consider how we can make our code more object-oriented rather than more Fortran-like. I believe we should replace wxPoint and wxRealPoint with KiCad-specific objects. These objects should be named such that they clearly indicate that they of represent locations rather than a simple array of integer values. They should be implemented such that nonsensical operations are prohibited and sanity checks put in place where possible. For example, attempting to add two locations should result in a compile-time error. Additional objects should be defined to represent the delta between two locations, and the operations of adding a delta to a location should result in a location. Just for discussion, let's assume the replacement for wxPoint is named KiPoint. The result of subtracting two KiPoint objects would be another object called KiDelta. Adding two KiPoint objects should be undefined, and result in a compile-time error, as this is a nonsense operation. Adding a KiPoint and a KiDelta should return a KiPoint. We should never abuse a KiPoint to represent anything else, such as using the X value as the radius of a circle as is currently done. I suspect most of the places where this would be much more than a mechanical substitution is a place where the code abuses the type or is a latent bug. Doing this now, before we go too far down the path of replacing wxWidgets types with non-OOB arrays would enhance readability and make the code more robust. Using VECTOR2I is going the wrong way. -Reece ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
