[3.x] CanvasItemEditor: Fix snapping grid misalignment

[2shady4u]

Cherry-picks changes from #70428 to Godot 3.X.

Bugsquad edit
Fixes #98466

Grid alignment is horrible broken in Godot 3.X and needs to be fixed asap, especially for pixelart games in which grid alignment is crucial.

[AThousandShips]

Could this not be cherry-picked automatically? We normally cherry pick to older branches in batches when appropriate

[2shady4u]

Could this not be cherry-picked automatically? We normally cherry pick to older branches in batches when appropriate

I didn't know automatic cherry-picking was a thing? 😄
But it has been two years since the original PR was created so I don't know why it was missed then? 🤔

(Work-around is to edit the editstate.cfg of the scene manually, but that becomes cumbersome fast.)

[AThousandShips]

Cherry picking as in git cherry-pick

But see the comment on the original PR for this #70428 (comment)

You also have to credit the original author, see adding co-authors

[2shady4u]

Cherry picking as in git cherry-pick

But see the comment on the original PR for this #70428 (comment)

You also have to credit the original author, see adding co-authors

Yeah, I used the cherry-pick command to cherry-pick the original commit from @phnix-dev.
I also added him as a co-author now 😃

[lawnjelly]

I had a little look at this yesterday with the view of moving this along, i.e. how to address the regression.

I'm not super familiar with the area so had to read some of the previous topics.

imo this PR should have a lot more information describing the original problem, why the PR doing the regression was introduced, and then more consideration in the PR "fixing" the regression (which this attempts to cherry pick).

It is understandable to some extent given that this is a cherry pick not written by the original author @phnix-dev . But I would like some more input from the original author / other 2d contributors and possibly @timothyqiu who cherry picked as he may be more familiar.

If I am correct in assessing the situation:

• Fix Range doesn't take into account the min value when setting the value. #65101 fixed a minor bug in slider ranges
• It was unforeseen that this could create regressions in existing sliders (especially in the editor)
• An obvious regression was seen in Godot 4 b8's UI-Ruler & grid getting corrupted and becomes inaccurate. #70186 which is "fixed" (or possibly patched over?) by this PR
• Possibly another regression: Godot 4 b8's UI-Ruler & grid getting corrupted and becomes inaccurate. #70186 (comment)

I can see that this has been the pattern in master, but we are generally a lot more conservative in 3.x as backward compatibility is paramount.

My concerns are that #65101 could potentially have created more than just 1 regression, in not just the editor, but potentially also in user projects, and we could end up in a "whack-a-mole" situation trying to fix these. This is feasible in 4.x, which has lots of testing and rapid change, but not so feasible in 3.x, which is in LTS and people are expecting existing code to work "as is".

So rather than just merging this fix, we have a potential choice between this, and simply reverting the original PR #65101 and living with the pre-existing bug (which I'm assuming has existed for years without a report, so is not major). These regressions do appear to be more major.

With a revert, we then have the opportunity for a new PR re-applying, but also fully considering all possible regressions, which ideally should have been the situation before merging the first PR.

Anyway, I'm not an expert here, just speaking from the responsibility of trying to keep the branch as stable as possible, and it may be that this can be guaranteed to be the only regression, but I think we should hear that from the author @phnix-dev .

Update

Having looked some more, I'm now tending towards thinking the original PR introducing the regressions (#65101 ) was likely a mistake, and the old version was better.

The reason is that a step in a control would normally be expected to orientate around zero (or is easier to understand that way). Orientating it around a minimum value (which is arbitrary) will create hard to predict side effects as seen in the regressions.

So I'm now leaning towards reverting #65101 (in 3.x). I'll bring this up in rocketchat to discuss in editor channel too.

[timothyqiu]

I think the intention is to only allow input to be min + step * X.

The original 0.01 min only allows values like 0.01, 1.01, 2.01, 3.01, etc after #65101. That's the cause of the issue/regression.

There is always an ambiguity when the min in the property hint is not divisible by step. For example, should the property hint 2,12,4 allow the use of 4, 8, 12 or 2, 6, 10? Both seem to make sense. The current behavior is more functional and can express both sets of values, while the previous behavior could not express the latter.

[lawnjelly]

But in turn this means that fractional minimums are no longer viable in a lot of cases.
Say you wanted to prevent divide by zero, but have otherwise a grid every 4th, previously you could use:

min 0.01, step 4

Now as I understand it, that is no longer possible?
So you are getting some flexibility in exchange for losing it elsewhere?

The other major concern is that this is not backward compatible, not just in the engine, but in third party code.

The backward compatibility is my major concern here. 🤔

Really to add what is a new ability, I wonder if we should try and add in a backward compatible way, such as introducing an additional "shift" for the range:

• min absolute minimum for the final result (as before)
• max absolute maximum for the final result (as before)
• step step based around zero (as before)
• shift default to zero, but apply before the step

This is a little more to think about, but retains backward compatibility and is flexible for both cases, and likely in most cases the shift would not be used.

[timothyqiu]

But in turn this means that fractional minimums are no longer viable in a lot of cases. Say you wanted to prevent divide by zero, but have otherwise a grid every 4th, previously you could use:

min 0.01, step 4

Now as I understand it, that is no longer possible?

For property hints, using 0.01 as a minimum in order to prevent the property being zero is a hack. It's not a feature of SpinBox / Range.

The number is arbitrarily chosen and only works in the inspector. To avoid zero seriously, it should be checked in the setter, then the inspector is zero-free automatically.

Update: I guess reverting that PR might be easier than updating related hints indeed.

[lawnjelly]

Update: I guess reverting that PR might be easier than updating related hints indeed.

Yes, I do worry that this could be a rat's nest and difficult to find all cases, and then we have third party addons / modules etc to consider.

And the "primary directive" for 3.x is as an LTS version - we should try and introduce new features / enhancements in a non-breaking way wherever possible.

I have no objections to adding it in a backward compatible way, but maybe that would be more appropriate as a new PR starting from the old working state.

Anyway we can leave this open for discussion so everyone interested can chime in on the best way to address (I certainly don't want to be making any unilateral decisions), there's no hurry until we are planning an imminent 3.6 point release.

This probably does warrant a point release as the 2d snapping grid is the most major bug so far afaik, but we should make sure any other possible pending bugs are fixed too.

[phnix-dev]

Hello 👋, I am the one who made this PR (#65101)
I sincerely apologize as I do not have the time to help fix these bugs. As @lawnjelly rightly said, Godot 3 is an LTS version and it is better if fixes do not introduce regressions. I think keeping this fix in Godot 4 and reverting it to Godot 3 might be a good solution. I believe the fix has led to regressions because contributors wrote their code based on math that was flawed to begin with. As such, it may be difficult to see and change everything that is wrong now.