Adding/Removing entries from an array causes the inspector to rebuild.

[MewPurPur]

Godot version

4.0.2

System information

Ubuntu 22.04.1 LTS

Issue description

In my curve editor overhaul PR #74959, I wanted to make it so LMB creates points and allows to adjust them before the mouse button is released. However, as soon as I add a point, the dictionary with the points adds a new entry, which causes a property tree update, reconstructing the whole inspector. This causes the point to become unselected, messing up the workflow in my PR. It's the only loose end remaining for that PR and I don't believe it's necessary, since Dictionary is handled by its own editor plugin.

Steps to reproduce

If you have Godot's source code, you can add print_line("test") to the CurveEditor constructor (curve_editor_plugin.cpp) and see that it prints every time points are added or removed.

Minimal reproduction project

N/A

[Rindbee]

Isn't it feasible to use set_meta()/get_meta() to (temporarily) cache the needed information (operation type, points, etc.) in the curve, and then use this information to restore the state after rebuilding the tree to continue operation?

[MewPurPur]

Yeah, I just want to document the bug.

Storing metadata is an ugly workaround for this, but I might wind up using it when I have time to get back to the PR if no one has fixed this yet.

[ajreckof]

Checked and the inspector is rebuilt because notify_property_list_changed is called when adding a point . You should wait until you have the final position to add the point.

[MewPurPur]

I didn't really want to discuss my particular PR, I just think it's quite unexpected that the whole inspector gets rebuilt when pairs inside a dictionary are added, as those are handled by a separate plugin. I believe if anything should rebuild at all, it should be the dictionary editor plugin.

But anyway, since we're onto discussing that particular PR...

The reason both of these are harder than they seem is because of an enum with three grab_mode states: NONE when no point is grabbed, ADD when a new point is being added and you're adjusting its position, and MOVE when an existing point is being moved. The ADD state has differences from the MOVE state:

• The point is added temporarily until you release the mouse button. Once you release, the temporary point is deleted and a real point is added, with an UndoRedo action and everything.
• When you use RMB, the point is deleted silently, as if you never added it. This is unlike the MOVE state, which moves the point to its old position silently. (Like in Blender and our 2D and 3D editors)

So disabling doesn't just unselect the point, it makes it so a point that was supposed to be temporary and without UndoRedo actions becomes the real point, as the curve editor forgets it was in the ADD state.

@Rindbee This could work. I'd have to activate it when new points are added and store the added point. Then when the curve editor is re-opened, it would need to check if this metadata exists, and if it does, it should change directly to the ADD state with this point selected, then delete the metadata. This should work alright, but it's quite heavy for a hacky workaround IMO, as I'd have to modify the code in multiple places far from each other and document it... Also metadata is sticky and has the potential to stick to users' curves - although it shouldn't happen in this scenario if it works as intended.

@ajreckof What you're suggesting is less hacky of a workaround, but it is way more complex. The reason for having a temporary point is for display. As you drag the temporary point around, you see what its final position would be, including any modification (clamping inside the range and domain, snapping), and it also allows you to see what the curve will look like. Mimicing all this would be very challenging without adding "true" points to the curve.

[ajreckof]

I didn't really want to discuss my particular PR, I just think it's quite unexpected that the whole inspector gets rebuilt when pairs inside a dictionary are added, as those are handled by a separate plugin. I believe if anything should rebuild at all, it should be the dictionary editor plugin.

I'm not sure what you mean by that as I don't understand where there is a pair added to a dictionnary. I would gladly try to solve it if I was able to understand. While working on the thing I wrote under I read #74927. And I think it gave me more context. So if i understand well this is about adding element to this

It took me some time to understand as I consider this an array (and right now I'm still unsure because of it). If this is the problem than this is expected behaviour as they are each one of them a property so notify_property_list_changed has to be called whenever one of them is added/removed. One way to still solve the problem would be to have a way to restrict the notify_property_list_changed for example only refreshing a category (using prefix) or having a regex to say what property are to be refreshed/added.

@ajreckof What you're suggesting is less hacky of a workaround, but it is way more complex. The reason for having a temporary point is for display. As you drag the temporary point around, you see what its final position would be, including any modification (clamping inside the range and domain, snapping), and it also allows you to see what the curve will look like. Mimicing all this would be very challenging without adding "true" points to the curve.

So Took a look and the behind was not how I thought it would look like. Based on the behind of Curve and CurveEditor I would suggest you rename _add_point to add_point_no_update and make it public than here you would just need to call add_point_no_update instead of add_point.

[MewPurPur]

🤦‍♀️ Oops

(I think dictionaries have the problem too though)

[MewPurPur]

About your suggestion, that might be the cleanest way yet! It can't have any bad effects and it's fairly simple, so I'll look into implementing it and adding a FIXME linking to this issue.

[MewPurPur]

If this bug gets fixed, please remember to see the FIXMEs from the PR above and address them.

[ajreckof]

Just a small heads up with Struct being in the work this might see a fix. Especially those Inspector Array could be replaced by Arrays of struct. This could have the same functionnality but would benefit from not needing a full Inspector rebuild when modifying the length of the array.