Re: [Kicad-developers] CPolyLine refactor

[Alejandro Garcia Montoro]

>
> Because we need mainly a class to store a polygon with holes (to replace
> CPolyLine) improving
> iterators and functions, especially to handle the first outline with holes
> can be an option.
>

This was my only concern, CPolyLine is a single polygon with holes and we
have no other class with the same exact meaning. I've got no problem in
replacing it with a set of just one polygon, if you agree.

I'm working right now on the Iterator class. Now there's an option to
iterate through the vertices of the hole or just through the vertices of
the outline. The possibility of iterating only the first polygon was
already implemented.
___
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] CPolyLine refactor

[jp charras]

Le 28/11/2016 à 15:37, Tomasz Wlostowski a écrit :
> On 23.11.2016 15:02, jp charras wrote:
>> Le 23/11/2016 à 13:32, Alejandro Garcia Montoro a écrit :
>>> Hi!
>>>
>>> Thanks for the tips :)
>>>
>>> I have some more doubts, one of them quite important: we have a class for 
>>> storing a set of polygons
>>> with holes, SHAPE_POLY_SET, but there is no explicit class to store a 
>>> single polygon with holes.
>>> There's a POLYGON typedef that could do the trick, but I think it would be 
>>> a better design if we
>>> created a SHAPE_POLYGON class that contained the methods from 
>>> SHAPE_POLY_SET (basically the
>>> iterator, the management of holes and vertices and the overrided from 
>>> SHAPE) and SHAPE_POLY_SET
>>> would be just a wrapper over SHAPE_POLYGON, basically a vector of them with 
>>> a couple of the methods
>>> that are now implemented (probably the ones from the Clipper library). 
>>> Furthermore, the
>>> SHAPE_POLY_SET::ITERATOR class should be also refactored and completed, as 
>>> for now it just iterates
>>> through the vertices of the outlines of the polygons, not through the 
>>> vertices of its holes.
>>>
>>> I think I could send a patch soon with these changes, before going ahead 
>>> with the work I'm doing for
>>> the CPolyLine refactor (which is somewhat deeper than I though on the first 
>>> time), as I see a better
>>> correspondence between CPolyLine and SHAPE_POLYGON than between the former 
>>> and SHAPE_POLY_SET.
>>>
>>> However, I still need a second opinion on this, I don't know if I'm 
>>> misunderstanding something.
>>>
>>> Best,
>>> Alejandro
>>
>> I think extracting POLYGON class from SHAPE_POLY_SET to make it a full 
>> separate class is a good idea.
>> Perhaps you could call it SHAPE_POLYGON_WITH_HOLE, to be more explicit and 
>> avoid mistakes with a
>> simple usual polygon.
>>
> 
> Hi JP & Alejandro,
> 
> I'm hesitant to agree with this idea:
> - We risk proliferating more polygon classes this way. Pcbnew operates
> almost exclusively on polygon sets. I can't think of a single case where
> a special case for a single polygon with holes would be beneficial. For
> storing polygon outlines, a SHAPE_LINE_CHAIN should be sufficient.

In fact this class exists, more or less in SHAPE_POLY_SET.
A SHAPE_LINE_CHAIN is perfect as long we want to store a polygon without holes.

Because we are talking about CPolyLine refactoring, this is not an option.
It is good to handle DRAWSEGMENTs of polygon type (which are expected to be 
simple polygons).

> - It's not possible to predict the result of boolean polygon operations
> compile-time, they return one or more polygons, each of them
> with/without holes.

Yes, this is true.

 Therefore, having specialized classes for each of
> these cases would make handling them unnecessarily complex (RTTI).
> - I would rather improve the iterators in the current SHAPE_POLY_SET
> class so that it's easy to access all vertices of each outline/hole.

This is also an option.

Because we need mainly a class to store a polygon with holes (to replace 
CPolyLine) improving
iterators and functions, especially to handle the first outline with holes can 
be an option.

> 
> Cheers,
> Tom
> 


-- 
Jean-Pierre CHARRAS

___
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] CPolyLine refactor

[Tomasz Wlostowski]

On 23.11.2016 15:02, jp charras wrote:
> Le 23/11/2016 à 13:32, Alejandro Garcia Montoro a écrit :
>> Hi!
>>
>> Thanks for the tips :)
>>
>> I have some more doubts, one of them quite important: we have a class for 
>> storing a set of polygons
>> with holes, SHAPE_POLY_SET, but there is no explicit class to store a single 
>> polygon with holes.
>> There's a POLYGON typedef that could do the trick, but I think it would be a 
>> better design if we
>> created a SHAPE_POLYGON class that contained the methods from SHAPE_POLY_SET 
>> (basically the
>> iterator, the management of holes and vertices and the overrided from SHAPE) 
>> and SHAPE_POLY_SET
>> would be just a wrapper over SHAPE_POLYGON, basically a vector of them with 
>> a couple of the methods
>> that are now implemented (probably the ones from the Clipper library). 
>> Furthermore, the
>> SHAPE_POLY_SET::ITERATOR class should be also refactored and completed, as 
>> for now it just iterates
>> through the vertices of the outlines of the polygons, not through the 
>> vertices of its holes.
>>
>> I think I could send a patch soon with these changes, before going ahead 
>> with the work I'm doing for
>> the CPolyLine refactor (which is somewhat deeper than I though on the first 
>> time), as I see a better
>> correspondence between CPolyLine and SHAPE_POLYGON than between the former 
>> and SHAPE_POLY_SET.
>>
>> However, I still need a second opinion on this, I don't know if I'm 
>> misunderstanding something.
>>
>> Best,
>> Alejandro
> 
> I think extracting POLYGON class from SHAPE_POLY_SET to make it a full 
> separate class is a good idea.
> Perhaps you could call it SHAPE_POLYGON_WITH_HOLE, to be more explicit and 
> avoid mistakes with a
> simple usual polygon.
> 

Hi JP & Alejandro,

I'm hesitant to agree with this idea:
- We risk proliferating more polygon classes this way. Pcbnew operates
almost exclusively on polygon sets. I can't think of a single case where
a special case for a single polygon with holes would be beneficial. For
storing polygon outlines, a SHAPE_LINE_CHAIN should be sufficient.
- It's not possible to predict the result of boolean polygon operations
compile-time, they return one or more polygons, each of them
with/without holes. Therefore, having specialized classes for each of
these cases would make handling them unnecessarily complex (RTTI).
- I would rather improve the iterators in the current SHAPE_POLY_SET
class so that it's easy to access all vertices of each outline/hole.

Cheers,
Tom



> Regards,
> 
>>
>> 2016-11-21 18:32 GMT+01:00 jp charras > >:
>>
>> Le 21/11/2016 à 18:07, Tomasz Wlostowski a écrit :
>> > On 21.11.2016 13:56, Alejandro Garcia Montoro wrote:
>> >> Hi!
>> >>
>> >> I'm working on the CPolyLine refactor into SHAPE_POLY_SET. Before 
>> coding
>> >> anything more, I want to discuss with you the changes I have to do, 
>> just
>> >> to be sure I'm not doing any useless work.
>> >>
>> >> The classes that use CPolyLine, and that have to be modified, are:
>> >>
>> >>   * ZONE_CONTAINER: This class has two CPolyLine members: m_Poly and
>> >> m_smoothedPoly. The latter is not used, but the former is the one
>> >> that has all the information about the zone. It's necessary to
>> >> carefully refactor this class, as the interface of CPolyLine and
>> >> SHAPE_POLY_SET is not exactly the same, e.g., the Hatch-related
>> >> functions are not yet implemented in SHAPE_POLY_SET or the code in
>> >> DrawWhileCreateOutline access the methods of the old 
>> CPOLYGONS_LIST
>> >> class.
>> >
>> > Hi Alejandro,
>>
>> m_smoothedPoly temporary stores the outline (from m_Poly) before 
>> building the filled areas in zone,
>> because the zone outline can be smoothed or chamfered before filling 
>> this zone.
>>
>> >
>> > If it's only about hatching, it's (as far as I know) only for the
>> > purpose of displaying a hatched zone outline (JP, please confirm). Then
>> > it belongs to the renderer (be it legacy or GAL), not to the polygon 
>> class.
>>
>> Yes, the hatches are only for rendering purposes.
>> (more easy to see the different zone outlines, especially when zones 
>> overlap, and more easy to see
>> complex outlines with holes).
>>
>>
>> >>   * BOARD, PCB_PAINTER and POINT_EDITOR: These classes use instances 
>> of
>> >> the ZONE_CONTAINER class, that still has methods that return
>> >> CPolyLine objects. The work on these classes should be more or 
>> less
>> >> straightforward when the refactor on the first one is finished.
>> > OK.
>> >>
>> >> I would like to set up some tests on these classes, just to be sure 
>> that
>> >> the refactor does not change anything. Does anyone have some tests 
>> using
>> >> these classes that I could reuse?
>> > I don't ;-)
>> >>
>>  

Re: [Kicad-developers] CPolyLine refactor

[jp charras]

Le 23/11/2016 à 13:32, Alejandro Garcia Montoro a écrit :
> Hi!
> 
> Thanks for the tips :)
> 
> I have some more doubts, one of them quite important: we have a class for 
> storing a set of polygons
> with holes, SHAPE_POLY_SET, but there is no explicit class to store a single 
> polygon with holes.
> There's a POLYGON typedef that could do the trick, but I think it would be a 
> better design if we
> created a SHAPE_POLYGON class that contained the methods from SHAPE_POLY_SET 
> (basically the
> iterator, the management of holes and vertices and the overrided from SHAPE) 
> and SHAPE_POLY_SET
> would be just a wrapper over SHAPE_POLYGON, basically a vector of them with a 
> couple of the methods
> that are now implemented (probably the ones from the Clipper library). 
> Furthermore, the
> SHAPE_POLY_SET::ITERATOR class should be also refactored and completed, as 
> for now it just iterates
> through the vertices of the outlines of the polygons, not through the 
> vertices of its holes.
> 
> I think I could send a patch soon with these changes, before going ahead with 
> the work I'm doing for
> the CPolyLine refactor (which is somewhat deeper than I though on the first 
> time), as I see a better
> correspondence between CPolyLine and SHAPE_POLYGON than between the former 
> and SHAPE_POLY_SET.
> 
> However, I still need a second opinion on this, I don't know if I'm 
> misunderstanding something.
> 
> Best,
> Alejandro

I think extracting POLYGON class from SHAPE_POLY_SET to make it a full separate 
class is a good idea.
Perhaps you could call it SHAPE_POLYGON_WITH_HOLE, to be more explicit and 
avoid mistakes with a
simple usual polygon.

Regards,

> 
> 2016-11-21 18:32 GMT+01:00 jp charras  >:
> 
> Le 21/11/2016 à 18:07, Tomasz Wlostowski a écrit :
> > On 21.11.2016 13:56, Alejandro Garcia Montoro wrote:
> >> Hi!
> >>
> >> I'm working on the CPolyLine refactor into SHAPE_POLY_SET. Before 
> coding
> >> anything more, I want to discuss with you the changes I have to do, 
> just
> >> to be sure I'm not doing any useless work.
> >>
> >> The classes that use CPolyLine, and that have to be modified, are:
> >>
> >>   * ZONE_CONTAINER: This class has two CPolyLine members: m_Poly and
> >> m_smoothedPoly. The latter is not used, but the former is the one
> >> that has all the information about the zone. It's necessary to
> >> carefully refactor this class, as the interface of CPolyLine and
> >> SHAPE_POLY_SET is not exactly the same, e.g., the Hatch-related
> >> functions are not yet implemented in SHAPE_POLY_SET or the code in
> >> DrawWhileCreateOutline access the methods of the old CPOLYGONS_LIST
> >> class.
> >
> > Hi Alejandro,
> 
> m_smoothedPoly temporary stores the outline (from m_Poly) before building 
> the filled areas in zone,
> because the zone outline can be smoothed or chamfered before filling this 
> zone.
> 
> >
> > If it's only about hatching, it's (as far as I know) only for the
> > purpose of displaying a hatched zone outline (JP, please confirm). Then
> > it belongs to the renderer (be it legacy or GAL), not to the polygon 
> class.
> 
> Yes, the hatches are only for rendering purposes.
> (more easy to see the different zone outlines, especially when zones 
> overlap, and more easy to see
> complex outlines with holes).
> 
> 
> >>   * BOARD, PCB_PAINTER and POINT_EDITOR: These classes use instances of
> >> the ZONE_CONTAINER class, that still has methods that return
> >> CPolyLine objects. The work on these classes should be more or less
> >> straightforward when the refactor on the first one is finished.
> > OK.
> >>
> >> I would like to set up some tests on these classes, just to be sure 
> that
> >> the refactor does not change anything. Does anyone have some tests 
> using
> >> these classes that I could reuse?
> > I don't ;-)
> >>
> >> Furthermore, some more instances of CPolyLine HATCH_STYLE enum are
> >> found. I am not sure that the hatch style should be inside
> >> SHAPE_POLY_SET, that contains just geometric information: what do you 
> think?
> > As said above, I don't think SHAPE_POLY_SET should contain hatching
> > functions.
> >
> > Best,
> > Tom
> >>
> >> Any comments on this will be appreciated.
> >>
> >> Regards,
> >> Alejandro
> 
> 
> --
> Jean-Pierre CHARRAS
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers 
> 
> Post to : kicad-developers@lists.launchpad.net 
> 
> Unsubscribe : https://launchpad.net/~kicad-developers 
> 
> More help   : 

Re: [Kicad-developers] CPolyLine refactor

[Alejandro Garcia Montoro]

Hi!

Thanks for the tips :)

I have some more doubts, one of them quite important: we have a class for
storing a set of polygons with holes, SHAPE_POLY_SET, but there is no
explicit class to store a single polygon with holes. There's a POLYGON
typedef that could do the trick, but I think it would be a better design if
we created a SHAPE_POLYGON class that contained the methods from
SHAPE_POLY_SET (basically the iterator, the management of holes and
vertices and the overrided from SHAPE) and SHAPE_POLY_SET would be just a
wrapper over SHAPE_POLYGON, basically a vector of them with a couple of the
methods that are now implemented (probably the ones from the Clipper
library). Furthermore, the SHAPE_POLY_SET::ITERATOR class should be also
refactored and completed, as for now it just iterates through the vertices
of the outlines of the polygons, not through the vertices of its holes.

I think I could send a patch soon with these changes, before going ahead
with the work I'm doing for the CPolyLine refactor (which is somewhat
deeper than I though on the first time), as I see a better correspondence
between CPolyLine and SHAPE_POLYGON than between the former and
SHAPE_POLY_SET.

However, I still need a second opinion on this, I don't know if I'm
misunderstanding something.

Best,
Alejandro

2016-11-21 18:32 GMT+01:00 jp charras :

> Le 21/11/2016 à 18:07, Tomasz Wlostowski a écrit :
> > On 21.11.2016 13:56, Alejandro Garcia Montoro wrote:
> >> Hi!
> >>
> >> I'm working on the CPolyLine refactor into SHAPE_POLY_SET. Before coding
> >> anything more, I want to discuss with you the changes I have to do, just
> >> to be sure I'm not doing any useless work.
> >>
> >> The classes that use CPolyLine, and that have to be modified, are:
> >>
> >>   * ZONE_CONTAINER: This class has two CPolyLine members: m_Poly and
> >> m_smoothedPoly. The latter is not used, but the former is the one
> >> that has all the information about the zone. It's necessary to
> >> carefully refactor this class, as the interface of CPolyLine and
> >> SHAPE_POLY_SET is not exactly the same, e.g., the Hatch-related
> >> functions are not yet implemented in SHAPE_POLY_SET or the code in
> >> DrawWhileCreateOutline access the methods of the old CPOLYGONS_LIST
> >> class.
> >
> > Hi Alejandro,
>
> m_smoothedPoly temporary stores the outline (from m_Poly) before building
> the filled areas in zone,
> because the zone outline can be smoothed or chamfered before filling this
> zone.
>
> >
> > If it's only about hatching, it's (as far as I know) only for the
> > purpose of displaying a hatched zone outline (JP, please confirm). Then
> > it belongs to the renderer (be it legacy or GAL), not to the polygon
> class.
>
> Yes, the hatches are only for rendering purposes.
> (more easy to see the different zone outlines, especially when zones
> overlap, and more easy to see
> complex outlines with holes).
>
>
> >>   * BOARD, PCB_PAINTER and POINT_EDITOR: These classes use instances of
> >> the ZONE_CONTAINER class, that still has methods that return
> >> CPolyLine objects. The work on these classes should be more or less
> >> straightforward when the refactor on the first one is finished.
> > OK.
> >>
> >> I would like to set up some tests on these classes, just to be sure that
> >> the refactor does not change anything. Does anyone have some tests using
> >> these classes that I could reuse?
> > I don't ;-)
> >>
> >> Furthermore, some more instances of CPolyLine HATCH_STYLE enum are
> >> found. I am not sure that the hatch style should be inside
> >> SHAPE_POLY_SET, that contains just geometric information: what do you
> think?
> > As said above, I don't think SHAPE_POLY_SET should contain hatching
> > functions.
> >
> > Best,
> > Tom
> >>
> >> Any comments on this will be appreciated.
> >>
> >> Regards,
> >> Alejandro
>
>
> --
> Jean-Pierre CHARRAS
>
> ___
> 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] CPolyLine refactor

[jp charras]

Le 21/11/2016 à 18:07, Tomasz Wlostowski a écrit :
> On 21.11.2016 13:56, Alejandro Garcia Montoro wrote:
>> Hi!
>>
>> I'm working on the CPolyLine refactor into SHAPE_POLY_SET. Before coding
>> anything more, I want to discuss with you the changes I have to do, just
>> to be sure I'm not doing any useless work.
>>
>> The classes that use CPolyLine, and that have to be modified, are:
>>
>>   * ZONE_CONTAINER: This class has two CPolyLine members: m_Poly and
>> m_smoothedPoly. The latter is not used, but the former is the one
>> that has all the information about the zone. It's necessary to
>> carefully refactor this class, as the interface of CPolyLine and
>> SHAPE_POLY_SET is not exactly the same, e.g., the Hatch-related
>> functions are not yet implemented in SHAPE_POLY_SET or the code in
>> DrawWhileCreateOutline access the methods of the old CPOLYGONS_LIST
>> class.
> 
> Hi Alejandro,

m_smoothedPoly temporary stores the outline (from m_Poly) before building the 
filled areas in zone,
because the zone outline can be smoothed or chamfered before filling this zone.

> 
> If it's only about hatching, it's (as far as I know) only for the
> purpose of displaying a hatched zone outline (JP, please confirm). Then
> it belongs to the renderer (be it legacy or GAL), not to the polygon class.

Yes, the hatches are only for rendering purposes.
(more easy to see the different zone outlines, especially when zones overlap, 
and more easy to see
complex outlines with holes).


>>   * BOARD, PCB_PAINTER and POINT_EDITOR: These classes use instances of
>> the ZONE_CONTAINER class, that still has methods that return
>> CPolyLine objects. The work on these classes should be more or less
>> straightforward when the refactor on the first one is finished.
> OK.
>>
>> I would like to set up some tests on these classes, just to be sure that
>> the refactor does not change anything. Does anyone have some tests using
>> these classes that I could reuse?
> I don't ;-)
>>
>> Furthermore, some more instances of CPolyLine HATCH_STYLE enum are
>> found. I am not sure that the hatch style should be inside
>> SHAPE_POLY_SET, that contains just geometric information: what do you think?
> As said above, I don't think SHAPE_POLY_SET should contain hatching
> functions.
> 
> Best,
> Tom
>>
>> Any comments on this will be appreciated.
>>
>> Regards,
>> Alejandro


-- 
Jean-Pierre CHARRAS

___
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] CPolyLine refactor

[Tomasz Wlostowski]

On 21.11.2016 13:56, Alejandro Garcia Montoro wrote:
> Hi!
> 
> I'm working on the CPolyLine refactor into SHAPE_POLY_SET. Before coding
> anything more, I want to discuss with you the changes I have to do, just
> to be sure I'm not doing any useless work.
> 
> The classes that use CPolyLine, and that have to be modified, are:
> 
>   * ZONE_CONTAINER: This class has two CPolyLine members: m_Poly and
> m_smoothedPoly. The latter is not used, but the former is the one
> that has all the information about the zone. It's necessary to
> carefully refactor this class, as the interface of CPolyLine and
> SHAPE_POLY_SET is not exactly the same, e.g., the Hatch-related
> functions are not yet implemented in SHAPE_POLY_SET or the code in
> DrawWhileCreateOutline access the methods of the old CPOLYGONS_LIST
> class.

Hi Alejandro,

If it's only about hatching, it's (as far as I know) only for the
purpose of displaying a hatched zone outline (JP, please confirm). Then
it belongs to the renderer (be it legacy or GAL), not to the polygon class.
>   * BOARD, PCB_PAINTER and POINT_EDITOR: These classes use instances of
> the ZONE_CONTAINER class, that still has methods that return
> CPolyLine objects. The work on these classes should be more or less
> straightforward when the refactor on the first one is finished.
OK.
> 
> I would like to set up some tests on these classes, just to be sure that
> the refactor does not change anything. Does anyone have some tests using
> these classes that I could reuse?
I don't ;-)
> 
> Furthermore, some more instances of CPolyLine HATCH_STYLE enum are
> found. I am not sure that the hatch style should be inside
> SHAPE_POLY_SET, that contains just geometric information: what do you think?
As said above, I don't think SHAPE_POLY_SET should contain hatching
functions.

Best,
Tom
> 
> Any comments on this will be appreciated.
> 
> Regards,
> Alejandro
> 
> 
> ___
> 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