kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

Hi Orson,

I hit exactly the same thig with the arc assistant, so I made a little
function: GAL::ResetTextAttributes(), next to GAL::SetTextAttributes(
EDA_TEXT*).

This should be sufficient to make it easy to "reset" GAL state when in
a drawing call that doesn't depend on prior state. Also, if you add a
text attribute field in future, adding to this function is easy.

Also provides a handy way to init attributes in GAL::GAL().

Cheers,

John

On Thu, Mar 30, 2017 at 6:10 AM, Maciej Suminski
<maciej.suminski@xxxxxxx> wrote:
> Hi,
>
> Thank you John, I have just committed your patch.
>
> There are basic Save()/Restore() functions, but IIRC they do not store
> the font settings, but are mostly used to maintain the transformation
> matrix.
>
> I am not sure about the best solution here. None of the drawing
> functions make any assumptions about the used drawing settings (color,
> line width, stroke/fill, etc.), so perhaps this should be the same for
> texts. Alternatively there could be a Reset() function, but anyway you
> are likely to set the text line width and size, so effectively you would
> just restore the attributes. I am open to discuss other ideas.
>
> Cheers,
> Orson
>
> On 03/29/2017 01:40 PM, John Beard wrote:
>> Sorry, that patch doesn't have the right comment. Please use this one.
>>
>> Thanks,
>>
>> John
>>
>> On Wed, Mar 29, 2017 at 7:37 PM, John Beard <john.j.beard@xxxxxxxxx> wrote:
>>> Hi,
>>>
>>> This resolves https://bugs.launchpad.net/kicad/+bug/1677210.
>>>
>>> This is caused because the GAL ruler tool didn't reset its text
>>> mirroring flag before drawing bitmap text. This means if the GAL had
>>> just drawn mirrored text, it would be set, but this is invalid for
>>> OpenGL bitmap fonts (and if it were, would still be incorrect).
>>>
>>> I also reset bold/italic, though bitmaps text doesn't seem to care
>>> about that at all.
>>>
>>> This fixes the bug. In the longer run, this is still not really ideal,
>>> as every draw-er on the GAL seems to need to reset every single
>>> parametert that it might need. If a parameter is added, all these
>>> "reset loci" need to be vetted for possibly adding or resetting that
>>> parameter.
>>>
>>> Would some kind of stack mechanism make sense here? Unrelated
>>> functions probably should start from a blank slate of some sort. Maybe
>>> a way to avoid unnecessary stack shuffling if you /know/ you don't
>>> need to reset GAL parameters and it's a performance hit to do so
>>> needlessly?
>>>
>>> Cheers,
>>>
>>> John
>>>
>>>
>>> _______________________________________________
>>> 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
>
> _______________________________________________
> 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

From f60a13673be3bf30f84fcdb3dc2b634310ba3b6c Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, 30 Mar 2017 15:44:38 +0800
Subject: [PATCH 1/2] Add reset text attributes function to GAL - use for arc
 tool

Independent drawing functions (that don't expect any particular state to
be set up fo them) on the GAL need to reset the GAL properties they
use. This adds GAL::ResetTextAttributes() to make this easier.

This is important, as failing to reset mirroring can cause asserts in
OpenGL.

This is used in the Ruler tool (which previously did it one attribute at
a time) and also the Arc layout assistant, which previously failed to
reset mirroring.

Also reset on GAL construction, as these members appear to be
uninitialised.
---
 common/gal/graphics_abstraction_layer.cpp | 18 ++++++++++++++++++
 common/preview_items/arc_assistant.cpp    |  2 ++
 common/preview_items/ruler_item.cpp       |  5 ++---
 include/gal/graphics_abstraction_layer.h  |  8 ++++++++
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/common/gal/graphics_abstraction_layer.cpp b/common/gal/graphics_abstraction_layer.cpp
index cf9ae16f6..ea839b32a 100644
--- a/common/gal/graphics_abstraction_layer.cpp
+++ b/common/gal/graphics_abstraction_layer.cpp
@@ -69,6 +69,9 @@ GAL::GAL( GAL_DISPLAY_OPTIONS& aDisplayOptions ) :
     forceDisplayCursor = false;
     SetCursorEnabled( false );
 
+    // Initialize text properties
+    ResetTextAttributes();
+
     strokeFont.LoadNewStrokeFont( newstroke_font, newstroke_font_bufsize );
 
     // subscribe for settings updates
@@ -146,6 +149,21 @@ void GAL::SetTextAttributes( const EDA_TEXT* aText )
 }
 
 
+void GAL::ResetTextAttributes()
+{
+     // Tiny but non-zero - this will always need setting
+     // there is no built-in default
+    SetGlyphSize( { 1.0, 1.0 } );
+
+    SetHorizontalJustify( GR_TEXT_HJUSTIFY_CENTER );
+    SetVerticalJustify( GR_TEXT_VJUSTIFY_CENTER );
+
+    SetFontBold( false );
+    SetFontItalic( false );
+    SetTextMirrored( false );
+}
+
+
 VECTOR2D GAL::GetTextLineSize( const UTF8& aText ) const
 {
     // Compute the X and Y size of a given text.
diff --git a/common/preview_items/arc_assistant.cpp b/common/preview_items/arc_assistant.cpp
index 1e3504b7d..43739a666 100644
--- a/common/preview_items/arc_assistant.cpp
+++ b/common/preview_items/arc_assistant.cpp
@@ -131,6 +131,8 @@ void ARC_ASSISTANT::ViewDraw( int aLayer, KIGFX::VIEW* aView ) const
     gal.SetIsStroke( true );
     gal.SetIsFill( true );
 
+    gal.ResetTextAttributes();
+
     // constant text size on screen
     SetConstantGlyphHeight( gal, 12.0 );
 
diff --git a/common/preview_items/ruler_item.cpp b/common/preview_items/ruler_item.cpp
index 629fc1d94..15af2f05c 100644
--- a/common/preview_items/ruler_item.cpp
+++ b/common/preview_items/ruler_item.cpp
@@ -238,9 +238,8 @@ void RULER_ITEM::ViewDraw( int aLayer, KIGFX::VIEW* aView ) const
     gal.SetIsStroke( true );
     gal.SetIsFill( false );
     gal.SetStrokeColor( PreviewOverlayDefaultColor() );
-    gal.SetFontBold( false );
-    gal.SetFontItalic( false );
-    gal.SetTextMirrored( false );
+
+    gal.ResetTextAttributes();
 
     // draw the main line from the origin to cursor
     gal.DrawLine( origin, end );
diff --git a/include/gal/graphics_abstraction_layer.h b/include/gal/graphics_abstraction_layer.h
index 9dced85aa..00e5bc94e 100644
--- a/include/gal/graphics_abstraction_layer.h
+++ b/include/gal/graphics_abstraction_layer.h
@@ -362,6 +362,14 @@ public:
     virtual void SetTextAttributes( const EDA_TEXT* aText );
 
     /**
+     * Reset text attributes to default styling
+     *
+     * Normally, custom attributes will be set individually after this,
+     * otherwise you can use SetTextAttributes()
+     */
+    void ResetTextAttributes();
+
+    /**
      * @brief Set the font glyph size.
      *
      * @param aGlyphSize is the new font glyph size.
-- 
2.12.0