[Python Bindings] Problems accessing built-in colors (KiCad 7)

Software
1

Short story:

  • SETTINGS_MANAGER.GetColorSettings(“_builtin_classic”) crashes
import pcbnew
sm = pcbnew.GetSettingsManager()
cs = sm.GetColorSettings("_builtin_classic")

Produces:

terminate called after throwing an instance of 'std::out_of_range'
  what():  _Map_base::at
Abortado

I think this is because the _builtin_classic is not yet defined. Because I can use any name found in ~/.config/kicad/7.0/colors/

Looking at the code I see COLOR_SETTINGS provides:

    /**
     * Constructs and returns a list of color settings objects based on the built-in color themes.
     * These color settings are not backed by a file and cannot be modified by the user.
     * This is expected to be called by SETTINGS_MANAGER which will take ownership of the objects
     * and handle freeing them at the end of its lifetime.
     * @return a list of pointers COLOR_SETTINGS objects containing the default color theme(s)
     */
    static std::vector<COLOR_SETTINGS*> CreateBuiltinColorSettings();

And this is used by SETTINGS_MANAGER::loadAllColorSettings(). This member isn’t available from Python, but is used by SETTINGS_MANAGER::ReloadColorSettings(), which is available.

  • SETTINGS_MANAGER::ReloadColorSettings() also crashes
import pcbnew
sm = pcbnew.GetSettingsManager()
cs = sm.ReloadColorSettings()

Produces:

/..../kicad/pcbnew/pcbnew.cpp(317): assert "process" failed in Pgm().
Segment violation

Am I missing something? I can currently compile KiCad 7 and I can do small changes to the SWIG stuff.

Long story

KiCad 7 can now really plot in color. KiCad 5/6 had a PLOT_CONTROLLER.SetColorMode(bool) that was quite useless. But now we have PCB_PLOT_PARAMS.SetBlackAndWhite(bool) that works.

But enabling colors from Python is useless because the default colors used by PCB_PLOT_PARAMS are useless (i.e. all layers white, so they are invisible).

But you can do:

        sm = pcbnew.GetSettingsManager()
        cs = sm.GetColorSettings()
        po.SetColorSettings(cs)
        po.SetBlackAndWhite(False)

And you get the user color settings. But in order to get some repeatable colors I wanted to use some of the built-in themes. Here is where I found that I couldn’t.

This isn’t high priority for me because KiBot can currently generate colored plots, but using a really complex method to decompose the layers in tracks, pads, vias, etc. giving correct colors to each part and then merging the resulting files. If the KiCad plotter can do it things will become much more simple.

P.S. Don’t tell me use the kicad-cli because I’m also doing various things that it can’t currently do, and I doubt it will ever do. One of the most relevant things is to render the solder mask similar to what you get in real life (positive and translucent)

2

BTW: How do I know that layer colors are white?

I added the COLOR_SETTINGS to swig and was able to print the colors using something like this:

        cs = po.ColorSettings()
        print(cs.GetColor(0).ToCSSString())
        print(cs.GetColor(44).ToCSSString())

The patch for this is trivial:

diff --git a/pcbnew/python/swig/pcb_plot_params.i b/pcbnew/python/swig/pcb_plot_params.i
index 1599b180ed..9b19c54659 100644
--- a/pcbnew/python/swig/pcb_plot_params.i
+++ b/pcbnew/python/swig/pcb_plot_params.i
@@ -25,6 +25,7 @@
  * @file pcb_plot_params.i
  */
 
+%include settings/color_settings.h
 %include plotprint_opts.h
 %include pcb_plot_params.h
3

You are not missing anything, that code is just not meant to be run from standalone python script.

If you look at loadAllColorSettings() it calls Pgm() to get environment variables which is only set when a kicad executable is run.

There are many ways to fix this but they all involve patching kicad code. If you are comfortable with submitting merge request upstream I can recommend both exposing COLOR_SETTINGS (you already figured that out) and splitting loadAllColorSettings() to LoadBuiltingColorSettings() and the rest. With first one being public so you can call it from scripts. Alternatively you can add LoadBuiltingColorSettings() to just swig wrapper so that the method exists in python but kicad code is untouched.

4

Thanks @qu1ck ! I’ll try to do it.

5

The way I would do this is to make sure the settings are loaded by the helper when starting standalone scripts. I would not expose the load method directly to scripts. It’s an internal detail that may change.

6

It makes sense to expose COLOR_SETTINGS itself since there is a SetColorSettings() on the plotter.

That alone will make CreateBuiltinColorSettings() available so maybe modifying settings manager code is not needed at all. Just create builtins and pass one of them directly to plotter.

7

I would rather avoid that if possible, the more things that get exposed, the more Python developers will be upset when we make breaking changes. We are not interested in converting things like this into an API surface that has stability.

8

Python devs are used to things breaking, it is understood that current API is not stable.

There is no expectation that everything that is possible in swig API will be converted. User engagement with plugins and scripts doing variety of things will show what is actually needed and what is not. If stable API will have completely different way to achieve the same thing, that’s fine. If it will have no way to do some things that users rely on it will hold back adoption of new API until it does.

But hampering old API for “this will generate more work” reasons seems shortsighted. It imposes no obligations on devs and yet will be a great way to help correctly prioritize features for new API.

9

I guess my point is, for every feature that you are looking for to do in Python today, the order of preference should be:

  1. Don’t expose it at all, make it be automatic (e.g. a Python script should not have to make a specific call in order to get the default colors loaded)

  2. Expose it at the highest possible level required, to minimize the chance that internal changes will break the interface. This means using techniques like creating manual wrappers to expose one or two methods from a class.

  3. If neither of those are practical, start exposing classes directly through SWIG. And when taking this route, make liberal use of %ignore to try to limit the things exposed to SWIG to those that are directly needed by a use case.

10

That makes perfect sense, thanks for clarifying.

Then probably built in color schemes should be created once in SETTINGS_MANAGER constructor and not be recreated in ReloadColorSettings(). This will make it automatic for scripts.

1 Like
11

Ok, I tried the first approach. So I added the built-in creation to the constructor, like this:

diff --git a/common/settings/settings_manager.cpp b/common/settings/settings_manager.cpp
index 9b610ef4f1..bf1b95f448 100644
--- a/common/settings/settings_manager.cpp
+++ b/common/settings/settings_manager.cpp
@@ -66,6 +66,10 @@ SETTINGS_MANAGER::SETTINGS_MANAGER( bool aHeadless ) :
 
     // create the common settings shared by all applications.  Not loaded immediately
     m_common_settings = RegisterSettings( new COMMON_SETTINGS, false );
+
+    // Create the built-in color settings
+    for( COLOR_SETTINGS* settings : COLOR_SETTINGS::CreateBuiltinColorSettings() )
+        m_color_settings[settings->GetFilename()] = RegisterSettings( settings, false );
 }
 
 SETTINGS_MANAGER::~SETTINGS_MANAGER()

And no more crashes from:

import pcbnew
sm = pcbnew.GetSettingsManager()
cs = sm.GetColorSettings("_builtin_classic")

Is this an acceptable patch?

12

It looks reasonable but I can’t test it this minute. Do you want to submit a MR for it?

13

Ok, I’ll try to create a MR.

1 Like
14

It will work but someone may come along later and see that defaults are created and then immediately recreated by call to ReloadColorSettings() and “optimize” away the one you added.

It’s probably better to modify ReloadColorSettings() to not recreate them.

15

The problem is the code calls clear() and removes all of them.
I can keep a copy, but I’ll submit the simplest patch and then see if I can it better.

16

IMHO the best would be to create them just once and reuse the objects during a reload.

But the code transfers the ownership to the unordered_map, so they get deleted by clear().

So instead of modifying this policy I tried the following: if the simple look-up in the dictionary fails I look if the name looks like an internal one (starts with _) and if the _builtin_default is not in the dictionary. In this case I assume we need to create the missing themes. So I create them and try the look-up again.

This ensures the built-in colors are there. This works for Python and also ensures that the assumption that we have _builtin_default is true.

Here is the patch:

diff --git a/common/settings/settings_manager.cpp b/common/settings/settings_manager.cpp
index 9b610ef4f1..c5a0d68e27 100644
--- a/common/settings/settings_manager.cpp
+++ b/common/settings/settings_manager.cpp
@@ -180,37 +180,50 @@ void SETTINGS_MANAGER::FlushAndRelease( JSON_SETTINGS* aSettings, bool aSave )
 COLOR_SETTINGS* SETTINGS_MANAGER::GetColorSettings( const wxString& aName )
 {
     // Find settings the fast way
     if( m_color_settings.count( aName ) )
         return m_color_settings.at( aName );
 
+    // Do we have the built-in colors?
+    // When using the Python API we could need to init them
+    if( aName[0] == '_' && !m_color_settings.count( "_builtin_default" ) )
+    {
+        // Create the built-in color settings
+        for( COLOR_SETTINGS* settings : COLOR_SETTINGS::CreateBuiltinColorSettings() )
+            m_color_settings[settings->GetFilename()] = RegisterSettings( settings, false );
+        // Try again
+        if( m_color_settings.count( aName ) )
+            return m_color_settings.at( aName );
+        // No luck, at least we won't fail to find "_builtin_default"
+    }
+
     // Maybe it's the display name (cli is one method of invoke)
     auto it = std::find_if( m_color_settings.begin(), m_color_settings.end(),
                             [&aName]( const std::pair<wxString, COLOR_SETTINGS*>& p )
                             {
                                 return p.second->GetName().Lower() == aName.Lower();
                             } );
 
     if( it != m_color_settings.end() )
     {
         return it->second;
     }
 
     // No match? See if we can load it
     if( !aName.empty() )
     {
         COLOR_SETTINGS* ret = loadColorSettingsByName( aName );
 
         if( !ret )
         {
             ret = registerColorSettings( aName );
             *ret = *m_color_settings.at( "_builtin_default" );
             ret->SetFilename( wxT( "user" ) );
             ret->SetReadOnly( false );
         }
 
         return ret;
     }
 
     // This had better work
     return m_color_settings.at( "_builtin_default" );
 }

This is automagic like @craftyjon suggested and the risk of inadverted optimization is low.

Should I change the MR?

17

It seems much simpler to me to simply not delete built in settings with clear() and instead selectively delete all others. Putting magic strings like “_builtin_default” into a constant somewhere is a good idea too.

But either way this discussion is getting way too technical for this forum and should be on gitlab.

18

Move to the MR? Open an issue? Which one is better?

19

Either one works, if you have a working solution that you want feedback on MR is easier to leave comments inline directly on code.

20

Ok, but I was thinking about the discussion.

I already moved the constant to a #define

But avoiding the clear() deletion needs messing much more with the code, I can try it, but only if @craftyjon thinks is a good idea. It also implies a much deeper testing to ensure we won’t leak memory. Years ago I used valgrin for this, but giving the size of KiCad … is the team using some memory debugger?