kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

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> 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>
>> 
> 
>