I’ve experienced this also. I assume it is due to how the API is generated. API just exposes C++ code without much consideration. And in this case I assume that posn is not a new object but just a reference to an existing object. Thus the issue is resolved when you explicitly create a new object which you modify.
Yes I agree with your inferences.
It would be interesting to know if SWIG has some settings to control whether it generates code to pass a reference, or to create a copy and pass a value.
Like I mentioned, it seems quite counterintuitive that you can modify an item’s position either by p = GetPosition() and then fiddling with p.x and p.y, or by using SetPosition().
I share your sentiment, but for the time being I think that with developer resources available to work on python API I am afraid we’re left on our own.
So for the this specific issue I think I found out the reason for this. If you look at how the position for PCB_TEXT::GetPosition() is derived you’ll see that it just takes what EDA_TEXT::GetTextPos() returns.
Looking further how EDA_TEXT::GetTextPos() is defined, you’ll notice it returns a reference ("&" character in the return value definition)
const wxPoint& GetTextPos() const { return m_e.pos; }
The upside of this is, that I’ll get familiar with C++ and sometime maybe I’ll even be able to contribute to the KiCad’s C++ code
It’s not counterintuitive to me. Swig or not, cpp or python, nowhere is it assumed that getters return a copy of an object. When you modify internals of a position object that will reflect on the drawing that uses that object as it’s position, makes perfect sense to me.
That’s a standard programming gotcha that you ran into, not specific to KiCad at all.
I am afraid we’re left on our own
I suspect so.
Your inspection of the C++ code supports our observations. However:
virtual wxPoint GetPosition() const override
shows that GetPosition is supposed to return a wxPoint object value, not a wxPoint& reference.
So I’m a bit puzzled how we seem to be getting a reference from GetPosition.
Yeah I am still getting familiar with C++. From from what I can gather, the PCB_TEXT::GetPosition()overloads BOARD_ITEM::GetPosition(), but the function returns what EDA_TEXT::GetTextPos()returns. And EDA_TEXT::GetTextPos() returns a reference. And then this reference is returned as a value. What happens in this case I don’t have a clue. I was assuming that it returns a reference that it got.
While the observed effect might have a roots in C++ code, it might as well be caused by python or SWIG.
As python is no stranger to issues with references:
a = [1, 2, 3]
b = [a, 4, 5, 6]
print(b)
>>> [[1, 2, 3], 4, 5, 6]
a[1] = 9
print(b)
>>> [1, 9, 3], 4, 5, 6]
It might also be an issue with just FP_TEXT. I did a quick test getting a possition of a footprint and a free text item and changing a a position that I got by calling GetPosition() and I did not observe what you got
import pcbnew
import wx
brd = pcbnew.GetBoard()
fp = brd.FindFootprintByReference('Q301')
fp.GetPosition()
wxPoint(69850000, 76200000)
pos = fp.GetPosition()
pos[0] = 0
pos
>>>wxPoint(0, 76200000)
fp.GetPosition()
>>>wxPoint(69850000, 76200000)
text_items = []
for t_i in brd.GetDrawings():
if isinstance(t_i, pcbnew.PCB_TEXT):
# text items are handled separately
text_items.append(t_i)
text_item = text_items[0]
tp = text_item.GetPosition()
tp
>>>wxPoint(243062295, 79130460)
tp[0] = 0
tp
>>>wxPoint(0, 79130460)
text_item.GetPosition()
>>>wxPoint(243062295, 79130460)
@MitjaN Well you prompted me to revisit the testing that convinced me that GetPosition() returned a reference.
Like you, I now cannot reproduce that symptom. Was my testing erroneous, or has something changed since the original testing?
Since that original observation I updated from Kicad 6.0.7 to 6.0.10. I guess that might make a difference. And I also became more familiar with what PCBNew fails to update data changes to the display, presenting opportunities for wrong observations. I would love to know which one
So for now, I think we should declare my original assertion that GetPosition() returns a reference as invalid for future purposes. Thanks for your effort to repro the problem!
When you modify internals of a position object that will reflect
on the drawing that uses that object as it’s position
Well, as it turns out MitjaN and I now can’t repro the symptoms that I could swear I saw, and fixed by creating a fresh wxPoint from the original wxPoint’s data.
I can’t completely repro my original test because I updated from 6.0.7 to 6.0.10 since then. And of course, I may have just plain misinterpreted my test results.
So it now appears that some_FP_TEXT.GetPosition() returns a wxPoint object, not a reference to the one internal to some_FP_TEXT.
This comports with my original expectation, and also with the API doc:
"wxPoint" GetPosition(self)
and source code:
virtual wxPoint GetPosition() const override
which, in my somewhat shakey understanding of C++ return syntax and implications, means returning an object. Unlike
const wxPoint& GetTextPos() const
which returns a reference to an object. But I’m prepared to be schooled to the contrary 
I checked commit history for v6 branch, nothing changed in that regard between 6.0.7 and 6.0.10. So the only explanation I have for what you observed is that you probably used GetTextPos() in your first test by mistake.
You are right that GetPosition() returns a copy. But it’s an outlier as it’s a small struct that’s cheap to copy. Normally getters in cpp will return references. If it’s a const reference then swig will actually make a copy for you but otherwise it’s the same object that is used by the parent. Always assume that modifying it will reflect in the parent unless you checked the cpp source.
Also about c++: I’m making a living by developing software for 16 years now, I still don’t know cpp. I’m just getting better at convincing other people that I do.
@qu1ck thanks for checking.
You might be right about accidentally using GetTextPos(). The API docs list methods not in alphabetical or related order, and also the available methods are scattered across FP_TEXT, BOARD_ITEM, EDA_ITEM, and EDA_TEXT, and for some text methods you do have to use a method that contains the segment “Text”, such as GetTextAngle and GetTextHeight. So it would be an easy slip to use GetTextPos() instead of GetPosition().
I did just try that out, and indeed:
ref = fp.Reference()
pos = ref.GetTextPos()
pos.x += 1000000
… does move the Reference Id. I noticed that PCBNew doesn’t immediately update the footprint appearance. If you select the footprint, then the Ref moves to the new position. Again, opportunities to make misleading ovbservations!
If it’s a const reference then swig will actually make a copy for you
Except
const wxPoint& GetTextPos() const { return m_e.pos; }
is an example const ref, and evidently Python doesn’t get a copy, but instead a reference to the original.
16 years now, I still don’t know cpp. I’m just getting better at
convincing other people that I do.
Hahaha, I have “learned C++” several times over past 40 years. Every time I come back to it there are whole new “improvements” and “deprecations of the one true way” and “cool” magic casting behaviors etc. These inevitably convince me that, while I have the confidence that I could probably understand C++ enough to get past the dangerous stage, it would be a race between escalating complexity and deteriorating mental capacity. The latter mostly brought on by the former.
I specifically checked the docs for this before posting as I wasn’t sure and thought that I got the answer but apparently
in most of the cases will be treated as a returning value
means “sometimes it will and sometimes it won’t, lol”.
Moral of the story: assume the worst and write defensive code.
1 Like
This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.