Refactor vector editor properties

[KoBeWi]

Follow-up to #75534 (comment)

The change I mentioned would require a tweak in all vector property classes and I didn't feel like doing the same thing 4 times, so I did a complete refactor. Instead of 6 classes that repeat literally the same thing there is only 1 now. The new class can handle any Vector with 2 or more components .-. Also, as a bonus, Vector4 now supports linking too.

Supersedes #75534
Closes #68165
Closes #73432

[KoBeWi]

This needs to be simplified, but it took me too long to even figure it out ;_;
Maybe later >_>

[Geometror]

How about this? It's longer but easier to read (command-query separation and reduced number of indentation levels). The name of rc1/rc2 could be more explicit, but it's late and I couldn't come up with something better :)

Suggested change

	if (p_name == COMPONENT_LABELS[i]) {
	for (int j = 0; j < ratio.size(); j++) {
	if (_get_ratio_component(j, 1) == i) {
	for (int k = 0; k < component_count - 1; k++) {
	spin_sliders[_get_ratio_component(j + k, 0)]->set_value_no_signal(spin_sliders[_get_ratio_component(j + k, 1)]->get_value() * ratio[j + k]);
	}
	break;
	}
	}
	// Find out which component has changed.
	int component_idx = -1;
	for (int i = 0; i < component_count; i++) {
	if (p_name == COMPONENT_LABELS[i]) {
	component_idx = i;
	}
	}
	int ratio_component_idx = -1;
	for (int j = 0; j < ratio.size(); j++) {
	int ratio_idx = _get_ratio_component(j, 1);
	if (ratio_idx == component_idx) {
	ratio_component_idx = j;
	}
	}
	// Update the other components based on the ratio.
	if (component_idx > 0 && ratio_component_idx > 0) {
	for (int k = 0; k < component_count - 1; k++) {
	int rc1 = _get_ratio_component(ratio_component_idx + k, 1);
	int rc2 = _get_ratio_component(ratio_component_idx + k, 0);
	double value = ratio[ratio_component_idx + k] * spin_sliders[rc1]->get_value();
	spin_sliders[rc2]->set_value_no_signal(value);
	}
	}

[YuriSizov]

This needs a rebase. And perhaps incorporating the suggestion.

[KoBeWi]

Rebased. I'm not convinced about the suggestion. It makes the code easier to read, but doesn't simplify the logic, which is convoluted and inefficient. I'll try to do something about it, but it's also something that can be done in a follow-up PR. For now I left a TODO comment.

The exact problem is that ratios are packed in an array that looks something like [ xy, yx, zy, yz, zx, zy] (I don't remember the exact order now xd), so it needs some advanced index magic to correctly map it. Of course the order is arbitrary, I just crammed all ratios into a single array and needed some way to do so. Maybe it can be changed to a HashMap or something.

[kleonc]

The exact problem is that ratios are packed in an array that looks something like [ xy, yx, zy, yz, zx, zy] (I don't remember the exact order now xd), so it needs some advanced index magic to correctly map it. Of course the order is arbitrary

@KoBeWi You can try something like this:

int n = component_count
int m = n - 1

// Updating sliders.
// slider[i].value = slider[j].value * ratios[k]
// (calculating k from i, j)
int j = changed_value_index
for i in n:
	if i == j:
		continue
	int k = m * j + ((i - j + n) % n) - 1
	slider[i].value = slider[j].value * ratio[k]

// Updating ratios.
// ratios[k] = slider[i].value / slider[j].value
// (calculating i, j from k)
for k in ratios.size(): // ratios.size() == n * m
	int j = k / m
	int i = (j + 1 + (k % m)) % n
	ratios[k] = slider[i].value / slider[j].value

It should result in ratios orderings (yx means y / x):

• [yx, xy],
• [yx, zx, zy, xy, xz, yz],
• [yx, zx, wx, zy, wy, xy, wz, xz, yz, xw, yw, zw].

[YuriSizov]

Something is wrong with linked values. I'm not sure if it's new to this PR or if it's a pre-existing issue, but it's an issue nonetheless. If you have a vector with components which have different signs, things get weird.

Sometimes the value and the sign of each component get in sync:

godot.windows.editor.dev.x86_64_2023-05-30_14-31-42.mp4

godot.windows.editor.dev.x86_64_2023-05-30_14-38-46.mp4

And sometimes it's only the values, but the signs are appropriately different:

godot.windows.editor.dev.x86_64_2023-05-30_14-40-19.mp4

It's a bit harder to test with non-integer vectors, as they are much slower to change, but they are not immune to the issue:

godot.windows.editor.dev.x86_64_2023-05-30_14-44-42.mp4

---

Note that it doesn't matter which component you drag, though initially I thought it did matter. What matters is how many times you cross the 0 threshold, and how the big numbers are (the bigger, the longer it takes them to sync).

[YuriSizov]

Other than that bug, everything seems to be working fine, btw, and I'm sure that code is way better to maintain now :)

[YuriSizov]

Okay, a correction. I only noticed this with negative numbers at first, so I assumed it was related, but indeed the only problem is that I was crossing the negative-positive boundary. The issue is repeatable even if you start with both positive numbers but a ratio different from 1:1. Crossing the 0 value with one of the components (maybe a few times) breaks the ratio.

KoBeWi says this is because when 0 is involved we can't have a valid ratio between components, which is reasonable. Perhaps we could cache the last valid ratio and use that?

[kleonc]

@YuriSizov Currently in EditorPropertyVectorN::_update_ratio if any of the sliders has zero value then all ratios will be set to 1.0. I think this should be changed and be resolved per component.

When dragging/changing value of a non-zero component, then all components can be scaled properly according to the ratios (at the start of the change), including the zero ones (they'll remain zero). So for (x, y, z) changing e.g. non-zero x to new_x would result in new_x * (1, y/x, z/x). Like for (1, 0, 2) changing x to 3 would result in (3, 0, 6). That's correct and expected and should be made like that.

The question is what the behavior should be when dragging/changing value of a zero component. I don't think there's an obvious expected behavior, it's rather a matter of making an arbitrary choice.

• According to the ratios what would be correct is setting other non-zero components to infinities. It might be not desired though. 🤔 E.g. for (1, 0, -2, 0) changing y to 3 would result in (INF, 3, -INF, 3) (could be argued it should result in(INF, 3, -INF, 0) instead).
• Another possibility would be to totally ignore linking for zero values. So for (1, 0, -2, 0) changing y to 3 would result in (1, 3, -2, 0) (or keep only the zeros linked so result would be (1, 3, -2, 3)😆).
• Maybe some other possibilities...

The decision on the behavior is needed before the implementation of course. 🙃

Also it should be ensured that e.g. when dragging the value in the inspector the _update_ratio is called only when the drag starts/ends. Haven't checked if it's currently like that but calling it in the middle of the drag could likely cause issues when going through zero (because of the different behavior), like the one mentioned in #77135 (comment).

[YuriSizov]

I like what you suggest and I like the idea of ignoring the ratio when starting the change from a zeroed component.

[KoBeWi]

I added tooltip and moved the properties to a new file.
I still did not touch the ratio logic. The bug you mentioned already existed before and this PR makes it easier to fix (as you need to do it only once xd).

[YuriSizov]

I opened a report for that problem: #77687

[YuriSizov]

Thanks!