pcbnew: wrong align items

[Eldar Khayrullin]

In some cases Align items works is wrong. Look next (used Align item to the top).

Application: kicad
Version: 4.1.0-alpha+201606101202+6909~45~ubuntu16.04.1-product, release build
Libraries: wxWidgets 3.0.2
           libcurl/7.47.0 OpenSSL/1.0.2g zlib/1.2.8 libidn/1.32 librtmp/2.3
Platform: Linux 4.4.0-24-generic x86_64, 64 bit, Little endian, wxGTK
- Build Info -
wxWidgets: 3.0.2 (wchar_t,wx containers,compatible with 2.8)
Boost: 1.58.0
Curl: 7.47.0
KiCad - Compiler: GCC 5.3.1 with C++ ABI 1009
        Settings: USE_WX_GRAPHICS_CONTEXT=OFF
                  USE_WX_OVERLAY=OFF
                  KICAD_SCRIPTING=ON
                  KICAD_SCRIPTING_MODULES=ON
                  KICAD_SCRIPTING_WXPYTHON=ON
                  USE_FP_LIB_TABLE=HARD_CODED_ON
                  BUILD_GITHUB_PLUGIN=ON

[Eldar Khayrullin (eldar)]

Sorry I hid fabrication layers.
Align operations use for align the Value field (on fabrication layer) and silk text too.
This is bug or intention?!
I think align operations should use for align only silk graphic (not text) and copper layers.

[Eldar Khayrullin (eldar)]

Correction for my previous message:
...
I think align operations shouldn't use for align the RefDes and Value fields (and maybe other text)

[Novak Tamas (novak-7)]

It seems a content box is calculated for each selected objects, and these rectangles will be aligned.
As rectangles with differently arranged Ref & Value are different shape and size, alignment seems very bad.
I think different behaviour needed: for whole footprint the container rectangle should be only the "body" of the component (without Ref and Value texts). Or even simpler the footprint would be represented with a point: its anchor.
Even more sophisticated approach needed for alignment of mixed selection (footprints, refs, values, tracks, vias) Do you have ideas for algorithm?

I think that actual solution is so bad (at least confusing) that is deserves a low-importance confirmed bug.

[Eldar Khayrullin (eldar)]

Why this bug is Incomplete? Maybe it deserves be Confirmed and be in Wishlist?!

[Chris Pavlina (pavlina-chris)]

Relax ;) It's Incomplete because he's waiting for someone else's opinion. Here, have an opinion.

[Eldar Khayrullin (eldar)]

To pavlina-chris, Ok :)

[Novak Tamas (novak-7)]

I dare to set "confirmed" as I dida new test:
- Click select a footprint (to left)
- Shift-Click select another footprint (2nd FP is rigth to 1st FP, and 2nd's Ref is the rightmost)
(now both footprints and both Refs are selected)
- Shift-Click to deselect both Refs
(now only footprint bodies are selected)

Now "Align items to the right" align 1st footprint to the right of the 2nd's Ref despite of that Ref is *unselected*. (Expected to align with the rihgt side of the 2nd footprint body)

[demidrol (dmitrodem-gmail)]

Well, maybe the patch attached could somehow be a solution. It adds items to 'Align/distribute' menu to make horizontal and vertical alignment possible. Alignment is done based on the 1st selected item (footprint, whatever).

[Ruth Ivimey-Cook (rivimey)]

I would like to chip in and say I too discovered this issue with the align tools, and on checking the code found the reason, too. I think the current implementation is broken.

IMO the align should most definitely not care about the silk-screen text, and probably should not care about the silk-screen or user layers at all.

I would suggest the only thing it should care about would be the copper layers, though I can see in case of things like connectors this *may* be the wrong thing to do. Perhaps including the Fab layer (as mentioned above) would address that concern. Either way this fact should be included in the docs.

I started playing around with the code, and the cause of the issue appears to be the GetBoundingBox() functions, which return the combined BB for all layers.

I think the fix would be to provide/extend a function to return the BB for all layers in a given class (e.g. copper, manuf, user). Perhaps a bitmask of interesting layers could be supplied to GetBoundingBox, which would default to 'all'?

[Chris Pavlina (pavlina-chris)]

I've been thinking about the bounding box methods - they're a bit inflexible in eeschema as well.

To me, the most flexible implementation would take a std::function<bool(EDA_ITEM*)> (lambda, etc) to arbitrarily filter which sub-elements are to be included. I've been thinking about implementing something like this.

Thoughts?

[Ruth Ivimey-Cook (rivimey)]

I wouldn't object to a 'lambda-function' approach, though I am not knowledgeable enough about kicad to understand why it would be more useful than a bitmask.

Mostly, I think we need something more than we have!

[Chris Pavlina (pavlina-chris)]

Using functors allow you to specify *any* criteria for filtering, not just one that was considered when the bitmask was written :)

[Robbert Lagerweij (rlagerweij)]

Fixed in:
https://git.launchpad.net/kicad/commit/?id=3556c4b8d6d5fe15e8b618acce3fb809a297bb8a