Path Command parameters are not mutable.

[FreeCAD-Bug-Importer]

Issue imported from https://tracker.freecad.org/view.php?id=4831

• Reporter: @sliptonic
• Date submitted: 1/16/2022
• FreeCAD version: 0.2
• Category: Bug
• Status: new
• Tags:

Original report text

The Path Command object is central to the path workbench. It contains all the axis words and parameters for a single line of pseudo gcode. However, the parameters are not mutable through the python interface. This often causes confusion, especially for new developers.

Additional information

comment by @iromero91 in pull request code review: https://github.com/FreeCAD/FreeCAD/pull/5229

DOH, that's the problem with mutable properties in wrapped objects, seems to always be a 50/50 if they are going to work properly or not. I'm a bit tempted to call this a bug in the Path.Command object, you should be expected to modify a mutable property and have it, you know, change. The workaround using the temp variable should be ok but we should consider fixing the wrapping later. There are a few solutions I would deem more acceptable than what we got now there.

• Remove the property interface and use something like getParameters() and setParameters() instead. To make it clear that objects are passed by-copy and you cannot mutate them.
• Make Parameters return a proxy object so that mutating it actually does the changes expected.
• Make Parameters return an immutable object and have a function like setParameter() to be able to write to them
• Remove the extra indirection and implement the parameters dictionary on the command class itself via __getitem__ and __setitem__, that way you could do p["F"] = 200. Also possible to implement the rest of the basic dictionary interface with keys() and items()

Steps to reproduce

>>> p = obj.Path.Commands[8]
>>> p
Command G3 [ F:0.8333 I:0 J:-2.5 K:0 X:-1.5 Y:76.038 Z:6 ]
>>> p.toGCode()
'G3 F0.833300 I0.000000 J-2.500000 K0.000000 X-1.500000 Y76.037957 Z6.000000'
>>> p.Parameters['F'] = 200
>>> p
Command G3 [ F:0.8333 I:0 J:-2.5 K:0 X:-1.5 Y:76.038 Z:6 ]
>>> p.Parameters
{'F': 200, 'I': 0.0, 'J': -2.5, 'K': 0.0, 'X': -1.5, 'Y': 76.037957, 'Z': 6.0}
>>> p.toGCode()
'G3 F0.833300 I0.000000 J-2.500000 K0.000000 X-1.500000 Y76.037957 Z6.000000'
>>> params = p.Parameters
>>> params['F'] = 200
>>> p.Parameters = params
>>> p
Command G3 [ F:200 I:0 J:-2.5 K:0 X:-1.5 Y:76.038 Z:6 ]
>>> p.toGCode()
'G3 F200.000000 I0.000000 J-2.500000 K0.000000 X-1.500000 Y76.037957 Z6.000000'
>>> 

FreeCAD Info

OS: Linux Mint 20 (i3/i3)
Word size of FreeCAD: 64-bit
Version: 0.20.27078 +6 (Git)
Build type: Unknown
Branch: feature/drill-refactor
Hash: 9ca8e01771210cbcc6159629dc675cb43bd4053b
Python version: 3.8.10
Qt version: 5.12.8
Coin version: 4.0.0
OCC version: 7.5.2
Locale: English/United States (en_US)

Other bug information

• Priority: normal
• Severity: minor
• Category: Bug
• Updated: 1/16/2022

[ShamanTcler]

So I am a newbie here, but I will start looking into this. Please do not let my involvement deter anyone else from solving his issue.

A few questions:

1. The issue was reported on Linux, has anyone replicated it on Windows? ... different compilers and all
2. I am considering the possibility that it was changed ... and then changed back .... thoughts?

Well off to continue to upgrade the tools and start a from scratch rebuild.

[sliptonic]

So I am a newbie here, but I will start looking into this. Please do not let my involvement deter anyone else from solving his issue.

Welcome. By all means. Please ask questions, make comments and try things.

1. The issue was reported on Linux, has anyone replicated it on Windows?  ... different compilers and all

Yes, the problem is the same on Windows. The problem has existed since Path started. It was even recognized then but since a work-around was possible, it wasn't a high priority to fix it.

2. I am considering the possibility that it was changed ... and then changed back .... thoughts?

Nope. It's always been there. I think there a few other places in FreeCAD where C++ datastructures aren't mutable through the python interface. I can't name any off the top of my head though.

[ShamanTcler]

So I have successfully built the Sliptonic repo of FreeCad ... but when I run the Python console I get a numpy error. (getting the right build env is always a pain in the but)

I downgraded my python to 3.8 the error says my numpy version is 1.19.3 ... any idea what it should be?

Edition Windows 11 Pro Version 22H2 Installed on ‎5/‎13/‎2022 OS build 22622.436 Experience Windows Feature Experience Pack 1000.22632.1000.0

[sliptonic]

Here's a better way to demonstrate the problem. Add these two tests to the end of PathTests/TestPathCore.py

    def test60(self):
        """Test Parameter Editing (indirect)"""
        p = Path.Command("G0 X100 Y100 Z0 F50")
        self.assertEqual(p.Parameters['F'], 50)

        # make a copy of params and change the copy
        params = p.Parameters
        self.assertEqual(params['F'], 50)
        params['F'] = 200
        self.assertEqual(params['F'], 200)

        # reassign the copy to the path parameters
        p.Parameters = params

        self.assertEqual(p.Parameters['F'], 200)
        self.assertEqual(p.toGCode(), "G0 F200.000000 X100.000000 Y100.000000 Z0.000000")


    def test70(self):
        """Test Parameter Editing (direct)"""
        p = Path.Command("G0 X100 Y100 Z0 F50")
        self.assertEqual(p.Parameters['F'], 50)

        # Directly change the parameter
        p.Parameters['F'] = 200
        self.assertEqual(p.Parameters['F'], 200)

        self.assertEqual(p.toGCode(), "G0 F200.000000 X100.000000 Y100.000000 Z0.000000")

test60 passes using the old style 'checkout' method. test70 currently fails because of the immutability bug.
Whoever fixes this, please commit the unit tests along with your PR.

[ShamanTcler]

I have spent some time learning the dev process and this bug. I have instrumented the code to determine the subroutine flow.

Using test 70 from above, things seem to be fine after "self.assertEqual(p.Parameters['F'], 50)"

I changed code to be:

        PathLog.notice("Python test70 after F=50 set, change it to 200\n")

        # Directly change the parameter
        p.Parameters['F'] = 200

        PathLog.notice("Python test70 after F=200 set\n")

Basically putting sentinels around the parameter set.

I also instrumented CommandPyImp.cpp so I logged each routine entrance/exit

For example:

PyObject *CommandPy::getCustomAttributes(const char* attr) const
{
    Base::Console().Log("CommandPy::getCustomAttributes() attr=%s\n", attr);
    std::string satt(attr);
    Base::Console().Log("     satt.length()=%d\n", satt.length() );
    if (satt.length() == 1) {
        if (isalpha(satt[0])) {
            boost::to_upper(satt);
            Base::Console().Log("     satt=%s\n", satt);
            if (getCommandPtr()->Parameters.count(satt)) {
                return PyFloat_FromDouble(getCommandPtr()->Parameters[satt]);
            }
            Py_INCREF(Py_None);
            return Py_None;
        }
    }
    Base::Console().Log("     Leaving getCustomAttributes\n");
    return nullptr;
}

The Log file nugget now looks like:

Log: TestPathCore.NOTICE: Python test70 after F=50 set

Log: CommandPy::getCustomAttributes() attr=Parameters
Log:      satt.length()=10
Log:      Leaving getCustomAttributes
Log: CommandPy::getParameters
Log:      End CommandPy::getParameters

Log: TestPathCore.NOTICE: Python test70 after F=200 set

So with satt.length = 10 ... it will never trigger the if block, basically a no-op.

Do I have this correct?

[agren]

Looks like getCustomAttributes does something like this: For any one-char attribute return p.Parameters[attribute]. So basically it makes it possible to write p.F instead p.Parameters['F'].

I would guess the Python <-> C++ integration tries to look up the attribute Parameters using getCustomAttributes (Since the word Parameters has length 10). Since getCustomAttributes returns nullptr it'll look somewhere else. And eventually end up running getParameters.

But I'm only guessing :)

[yorikvanhavre]

The problem is that the get/set interface expects you to modify a whole attribute, for example if p.Parameters is a list, you can get a list or set a list. Not set a subelement of that list.

This is a problem met elsewhere in FreeCAD, I'm not really sure how to approach it. When you do p.Parameters['F'] = 200, you are basically doing: Set 200 to the element F of the result of operation p.Parameters, which actually creates each time a new Python list from C++ contents. The python list is a representation of an internal C++ list, not the list itself.

So I'm not sure what should actually happen there, since that python list is actually an independent copy of the C++ one, and I see no way to do it otherwise.. But I'm no C++ expert, and the C++/Python interface is a whole world on its own, there might be something I'm not aware of..

[agren]

Maybe it's possible to create a class that has the same interface as a Python dict but is backed by a C++ std::map. Then you could return an instance backed by the C++ parameters map. Instead of returning a dict with copies of the parameters.

[iromero91]

@yorikvanhavre @agren that is what i was saying with my coment above. If you return a proxy object that implements the dict interface without being a "real" python dict it would work the way the python programmer would expect. You can even go further and implement the dictionary interface on the parent object itself. it is literally implementing 2 or 3 methods with magic names. Python will do the right thing.