kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Tom and Orson,

You are far more familiar with this code that I am.  I'm going to defer
to you for input and approval on this.  It's presently looking more
involved than I am comfortable with during a feature freeze.  Please let
me know what you think when you get a chance to review and test it.

Thanks.

Wayne

On 1/9/2018 3:29 PM, Jeff Young wrote:
> Hi Seth,
> 
> Yeah, I did try some of those other methods first.  But they all let
> implementation details bleed into the wrong places (COLLECORS_GUIDE
> instead of SELECTION_TOOL, but the info still doesn’t belong outside of
> the router).
> 
> You are correct that some of the changes in selectPoint are just cleanup
> and are not required.  I almost removed the Wait(TOOL_EVENT) thing
> entirely as selectPoint is never called with aOnDrag = true.  But that
> seemed like perhaps too much cleanup. ;)
> 
> However, it’s not true that Wait will get called under different
> circumstances as the heuristics part isn’t a filter per se, and won’t
> ever remove everything.
> 
> Cheers,
> Jeff.
> 
> 
>> On 9 Jan 2018, at 20:09, Seth Hillbrand <seth.hillbrand@xxxxxxxxx
>> <mailto:seth.hillbrand@xxxxxxxxx>> wrote:
>>
>> Jeff-
>>
>> I get your reasoning.  However, it seems like you could accomplish the
>> same goal (preventing disambiguation) by using the existing
>> GENERAL_COLLECTOR::Modules[] for your footprint disambiguation and
>> maybe modifying the COLLECTORS_GUIDE or Inspect() to get the track
>> disambiguation.  This would reduce the impact of your patch and not
>> create duplicate functionality.
>>
>> In other aspects, the changes to selectPoint do not seem required
>> (correct me if I'm wrong) and they introduce a subtle bug by
>> triggering a Wait(TOOL_EVENT) even if the heuristics have removed all
>> items from the collector.
>>
>> Overall, I like the idea.  It makes sense to have a post-filter.  But
>> I would worry that this is more invasive than is needed to resolve the
>> issue during feature-freeze.
>>
>> -S
>>
>> 2018-01-09 11:36 GMT-08:00 Jeff Young <jeff@xxxxxxxxx
>> <mailto:jeff@xxxxxxxxx>>:
>>
>>     Hi Seth,
>>
>>     The test_window creates a  SELECTION_TOOL without the rest of
>>     kicad behind it.  Since I added methods which SELECTION_TOOL
>>     calls, I had to mock them in the test files.
>>
>>     GENERAL_COLLECTOR::Modules[] can’t be used because the creator of
>>     the collector (SELECTION_TOOL) shouldn’t know anything about the
>>     semantics of the filter (only the specific action implementation
>>     knows that).  In the FootprintFilter case it’s simply enough that
>>     you’d nearly be willing to overlook that, but the other filters
>>     are more complicated and required things like ROUTER_TOOL
>>     knowledge, which definitely shouldn’t be leaking into SELECTION_TOOL.
>>
>>     Cheers,
>>     Jeff.
>>
>>
>>>     On 9 Jan 2018, at 19:26, Seth Hillbrand <seth.hillbrand@xxxxxxxxx
>>>     <mailto:seth.hillbrand@xxxxxxxxx>> wrote:
>>>
>>>     Same here.  Same errors.  Yes, cmaked.
>>>
>>>     I don't understand why you are changing the test_window link
>>>     libraries in this patch.
>>>
>>>     Also, why did you decide to write a "FootprintFilter" routine
>>>     instead of using the GENERAL_COLLECTOR::Modules[]?
>>>
>>>     -S
>>>
>>>     2018-01-09 11:05 GMT-08:00 Jeff Young <jeff@xxxxxxxxx
>>>     <mailto:jeff@xxxxxxxxx>>:
>>>
>>>         Did you do a CMake?  There’s a change in the CMake files….
>>>
>>>         > On 9 Jan 2018, at 18:59, kristoffer Ödmark
>>>         <kristofferodmark90@xxxxxxxxx
>>>         <mailto:kristofferodmark90@xxxxxxxxx>> wrote:
>>>         >
>>>         > Hmm, cannot compile, get a lot of undefiner references.
>>>         Even after nuking the build dir.
>>>         >
>>>         >
>>>         > ../../common/libpcbcommon.a(class_pad.cpp.o): In function
>>>         `D_PAD::HitTest(EDA_RECT const&, bool, int) const':
>>>         > kicad/pcbnew/class_pad.cpp:982: undefined reference to
>>>         `TestPointInsidePolygon(wxPoint const*, int, wxPoint const&)'
>>>         > ../../common/libpcbcommon.a(class_zone.cpp.o): In function
>>>         `ZONE_CONTAINER::Hatch()':
>>>         > kicad/pcbnew/class_zone.cpp:1219: undefined reference to
>>>         `FindLineSegmentIntersection(double, double, int, int, int,
>>>         int, double*, double*, double*, double*, double*)'
>>>         > ../../common/libcommon.a(shape_poly_set.cpp.o): In function
>>>         `SHAPE_POLY_SET::convertToClipper(SHAPE_LINE_CHAIN const&,
>>>         bool)':
>>>         > kicad/common/geometry/shape_poly_set.cpp:466: undefined
>>>         reference to
>>>         `ClipperLib::Orientation(std::vector<ClipperLib::IntPoint,
>>>         std::allocator<ClipperLib::IntPoint> > const&)'
>>>         > kicad/common/geometry/shape_poly_set.cpp:467: undefined
>>>         reference to
>>>         `ClipperLib::ReversePath(std::vector<ClipperLib::IntPoint,
>>>         std::allocator<ClipperLib::IntPoint> >&)'
>>>         > ../../common/libcommon.a(shape_poly_set.cpp.o): In function
>>>         `SHAPE_POLY_SET::booleanOp(ClipperLib::ClipType,
>>>         SHAPE_POLY_SET const&, SHAPE_POLY_SET::POLYGON_MODE)':
>>>         > kicad/common/geometry/shape_poly_set.cpp:489: undefined
>>>         reference to `ClipperLib::Clipper::Clipper(int)'
>>>         >
>>>         >
>>>         > On 2018-01-09 19:22, Wayne Stambaugh wrote:
>>>         >> Does the patch resolve your issue?
>>>         >>
>>>         >> On 1/9/2018 1:21 PM, kristoffer Ödmark wrote:
>>>         >>> Yes, I can reproduce this with very thick tracks!
>>>         >>>
>>>         >>>
>>>         >>> On 2018-01-09 16:55, Jeff Young wrote:
>>>         >>>> Hi Kristoffer,
>>>         >>>>
>>>         >>>> That’s odd.  Did you try it with your mouse pointer
>>>         directly over the
>>>         >>>> corner?  (You may need a reasonably thick track for this
>>>         to happen.
>>>         >>>> Try something on the order of 2mm.)
>>>         >>>>
>>>         >>>> Without my change the selection disambiguation is run
>>>         *before* we know
>>>         >>>> it’s a drag action on a simple corner, so the
>>>         selection_tool thinks it
>>>         >>>> needs to know which of the two segments is to be selected.
>>>         >>>>
>>>         >>>> Cheers,
>>>         >>>> Jeff.
>>>         >>>>
>>>         >>>>
>>>         >>>>> On 9 Jan 2018, at 15:37, Kristoffer Ödmark
>>>         >>>>> <kristofferodmark90@xxxxxxxxx
>>>         <mailto:kristofferodmark90@xxxxxxxxx>> wrote:
>>>         >>>>>
>>>         >>>>> If i understand him correctly that would only be when
>>>         on a corner, I
>>>         >>>>> think it would be the desired behaviour.
>>>         >>>>>
>>>         >>>>> Although, when I try it on my build on my laptop, there
>>>         is no clarify
>>>         >>>>> selection menu popping up when trying do drag ( 'd' )
>>>         or such on
>>>         >>>>> corners. Not in opengl anywas. So I dont know.
>>>         >>>>>
>>>         >>>>> Application: kicad
>>>         >>>>> Version: (2017-12-30 revision b55eb0b5f)-master,
>>>         release build
>>>         >>>>> Libraries:
>>>         >>>>>     wxWidgets 3.0.3
>>>         >>>>>     libcurl/7.57.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4
>>>         >>>>> libpsl/0.19.1 (+libidn2/2.0.4) libssh2/1.8.0 nghttp2/1.29.0
>>>         >>>>> Platform: Linux 4.14.12-1-MANJARO x86_64, 64 bit,
>>>         Little endian, wxGTK
>>>         >>>>> Build Info:
>>>         >>>>>     wxWidgets: 3.0.3 (wchar_t,wx containers,compatible
>>>         with 2.8) GTK+
>>>         >>>>> 2.24
>>>         >>>>>     Boost: 1.65.1
>>>         >>>>>     Curl: 7.57.0
>>>         >>>>>     Compiler: GCC 7.2.1 with C++ ABI 1011
>>>         >>>>>
>>>         >>>>> Build settings:
>>>         >>>>>     USE_WX_GRAPHICS_CONTEXT=OFF
>>>         >>>>>     USE_WX_OVERLAY=OFF
>>>         >>>>>     KICAD_SCRIPTING=ON
>>>         >>>>>     KICAD_SCRIPTING_MODULES=ON
>>>         >>>>>     KICAD_SCRIPTING_WXPYTHON=ON
>>>         >>>>>     KICAD_SCRIPTING_ACTION_MENU=ON
>>>         >>>>>     BUILD_GITHUB_PLUGIN=ON
>>>         >>>>>     KICAD_USE_OCE=ON
>>>         >>>>>     KICAD_SPICE=ON
>>>         >>>>>
>>>         >>>>>
>>>         >>>>> -Kristoffer
>>>         >>>>>
>>>         >>>>> On 01/09/2018 04:27 PM, Wayne Stambaugh wrote:
>>>         >>>>>> Jeff,
>>>         >>>>>> Have actually confirmed that this is the desired
>>>         behavior for this
>>>         >>>>>> outside of you own objectives?  I'm not saying that
>>>         this is or isn't a
>>>         >>>>>> good idea but I personally don't drag trace corners
>>>         around so I'm not
>>>         >>>>>> sure what the appropriate behavior should be.  You
>>>         should get comments
>>>         >>>>>> from the dev list and users before you make a change
>>>         like this.  As far
>>>         >>>>>> as pushing this to the dev repo, if it's not too
>>>         invasive I will
>>>         >>>>>> consider it.  If it is a large change set, I would
>>>         prefer that we hold
>>>         >>>>>> off until after the stable release.
>>>         >>>>>> Thanks,
>>>         >>>>>> Wayne
>>>         >>>>>> On 1/8/2018 5:49 AM, Jeff Young wrote:
>>>         >>>>>>> Wayne, if I could get you to don that old project
>>>         manager’s hat one
>>>         >>>>>>> more time:
>>>         >>>>>>>
>>>         >>>>>>> If we’re still weeks out from declaring an RC, I
>>>         wanted to make one
>>>         >>>>>>> more plug for getting rid of the Clarify Selection
>>>         dialog when
>>>         >>>>>>> dragging corners or using ‘U’ or ‘I’ over a corner[1].
>>>         >>>>>>>
>>>         >>>>>>> While it’s marked Wishlist, it seriously impacts
>>>         productivity when
>>>         >>>>>>> editing tracks, and I think most users would consider
>>>         it a bug
>>>         >>>>>>> (particularly in the corner case when dragging the
>>>         corner is
>>>         >>>>>>> clearly moving both the tracks listed in the Clarify
>>>         Selection menu).
>>>         >>>>>>>
>>>         >>>>>>> I’ve been running the patch for about a week now with
>>>         no issues.
>>>         >>>>>>>
>>>         >>>>>>> Cheers,
>>>         >>>>>>> Jeff.
>>>         >>>>>>>
>>>         >>>>>>> [1] https://bugs.launchpad.net/kicad/+bug/1708869
>>>         <https://bugs.launchpad.net/kicad/+bug/1708869>
>>>         >>>>>>> _______________________________________________
>>>         >>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>         >>>>>>>
>>>         >>>>>> _______________________________________________
>>>         >>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>>> More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>         >>>>> _______________________________________________
>>>         >>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         >>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>>>> More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>         >>>
>>>         >>> _______________________________________________
>>>         >>> Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         >>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >>> More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>         >> _______________________________________________
>>>         >> Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         >> Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         >> More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>         >
>>>         >
>>>         > _______________________________________________
>>>         > Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         > Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         > More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>
>>>
>>>         _______________________________________________
>>>         Mailing list: https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>         Unsubscribe : https://launchpad.net/~kicad-developers
>>>         <https://launchpad.net/~kicad-developers>
>>>         More help   : https://help.launchpad.net/ListHelp
>>>         <https://help.launchpad.net/ListHelp>
>>>
>>>
>>
>>
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>