Godot: A small fractional value is being juggled around 2D matrices, and can serialize unusual scene property values.

[avencherus]

Godot version:
8e91d2746d28d1f84f872ef535b55807e1ac3535

OS/device including version:
Win10 64-bit

Issue description:

Introduced with skew via: https://github.com/godotengine/godot/pull/38394

I just recently was doing a merge on some new work being done in our project, and there are a whole lot of skew properties showing up in the differences.

Are these harmful to have or do I have manually clear them out? Will these keep happening?

Why would they initially be anything different than the default of 0?

In one result it can be seen it creates a scale(1,1) entry, where there is no scaling, and that is the default value given by the editor. (The intended behavior seems to be to not serialize values that match the default values.)

Steps to reproduce:
I haven't studied it closely enough to know the numerical conditions and outcomes.

Minimal reproduction project:
N/A

[Ev1lbl0w]

If you open the file in the editor and it's not very different, then yeah, it's fine. Godot tends to change a lot of properties when you're editing scenes or fiddling with options, and all of those show up on VCS systems.

[groud]

They are due to the new skew property addition, so the new property is expected.

The fact the value is not zero is likely due to floating point values inaccuracy. This is kind of expected, since the position/rotation/skew values are extracted from a transform matrix and not stored as is in the object.

[avencherus]

Thanks for the responses.

@Ev1lbl0w Nothing was directly changed on these properties. 20 files were updated, and 640 nodes got skew values of 4.3711re-08.

@groud It's expected if a value is changed. When things are at default, the .tscn file from my experience never adds entries for unchanged properties. This clogs up the git history. Would there be any side effects to making "nearly zero" values 0.0?

[groud]

Rounding up values close to zero will mean that, internally, you would slightly modify the transform matrix.
As a consequence, this might cause the other values (like rotation, scale and position) to be slightly modified in return too. So basically, your skew would be a true zero but your rotation would be something lile 90.0000001 degrees.

[Calinou]

Can we avoid serializing the value if it's equal to 4.37114e-08? This way, we'd keep the matrix unmodified.

[groud]

Can we avoid serializing the value if it's equal to 4.37114e-08? This way, we'd keep the matrix unmodified.

Well, no. When you reimport the scene, if the value is missing it's going to be treated as zero. Then, internally, the reconstructed matrix will have very slight differences with the original values, which then might cause the imported properties to slightly diverge from the original ones. So it's the basically the same as setting the value to zero before the serialization step.

We could try setting the value to zero though, maybe the effect wont be visible, but I really doubt about it.

[avencherus]

I would think that having the tiny fraction appear on values that exist would be better, rather than create hundreds of new properties to store it.

[avencherus]

An additional wrinkle to this. It also seems to be adding and removing scale(1, 1) in some cases.

We had merge conflicts, because some scenes were unexpectedly modified in review, and accidentally added to a commit (lot of files with the same a similar name). So when I looked it over I saw things like this happening.

I'm going to revert this one on my end, since we don't use skew, and it has the potential to waste dev time.

[akien-mga]

I'll revert the skew addition from the 3.2 branch if we don't find a proper workaround to avoid these changes.

[Ev1lbl0w]

I don't know if Godot does this already, but isn't there a lazily way to save these parameters? As in, only save a variable that is different from the default value.

[akien-mga]

@Ev1lbl0w That's already what happens, but here skew = 4.3711e-08 is different from the default value (0).

As for scale = Vector2( 1, 1 ), I guess it might be some precision error that makes the engine see it as different from the default value (e.g. it's Vector2( 1, 0.9999999927 )) but when serializing it ends up rounded to 1 anyway.

[Zireael07]

Continuing from what @Ev1lbl0w said, maybe Godot could ignore very slight differences on certain properties (namely default, not-script created variables) in the range of +/- some epsilon? That would likely fix the current 4.3711e-08 vs 0 issue

[groud]

I'd rather round the values close to the default one (with epsilon comparison) than reverting the commit.

[Ev1lbl0w]

I agree that rounding the values is a way better solution than to revert a feature (especially because the problem OP is having is these values showing up on the files even though he's not using them, and not that this feature is causing issues on the project)

[avencherus]

If a solution is decided and implemented before stable release. 👍

If not, it could represent a situation for those who value or rely on version control. I've only suffered it for a day before pruning it. So I don't know the triggering mechanism or the full extent of how often it will dirty values in scene files.

Maybe it's a one time thing, or maybe it haunts a repo and periodically clouds up ones diffs with dozens to hundreds of unnecessary entries.

@Ev1lbl0w I can appreciate that this might be a feature you really are looking forward to, but the vcs impact isn't trivial. Below is a GIF giving a look at what one of our scene commits looks like, and this affects at least 20 scenes (There are dozens more, but we avoided opening them until reverting this.)

It's very difficult to find the actual changes that were made, or if there were any.

[Ev1lbl0w]

@avencherus Yes, I understand your point, and I agree; Godot tends to modify the scene files in ways that clog up VCS history. I work on remote projects too and I notice this a lot; someone changes a script, and in the commit history there are other 20 random scene files with changes, and we're not sure if these scenes we're actually changed, or just the editor who fiddled with some values.

A perfect example is the editor/display_folded, which is responsible for storing whether a node is collapsed or not:

This property gets constantly changed, and pollutes the git history with unnecessary changes.

So, I agree that this behavior should be definitely addressed; however, I don't think the solution to this would be to revert this new feature. This would only be a temporary fix; later things are added to the engine, and this problem happens again. The issue is not that this feature was added; it's that Godot tends to not be very consistent when handling scene files, and introduces a lot of changes when editing files.

Now, I don't know if this is easy to change, but we could try reducing unnecessary changes to the scene files, but I think this issue will always be present in some way.

[Calinou]

@Ev1lbl0w The scene folding status should no longer saved to scene files since Godot 3.2.

[Ev1lbl0w]

@Ev1lbl0w The scene folding status should no longer saved to scene files since Godot 3.2.

Wow, I'm quite happy to know that :smiley:
This was, to me, the most annoying property popping up in our code reviews, so that's a big improvement.

[akien-mga]

I've tried saving a few scenes with 3.2.2-rc1 in https://github.com/KOBUGE-Games/jetpaca and couldn't reproduce the issue.

Could you narrow it down to specific situations where the epsilon-esque skew gets added to the scene file?

[avencherus]

I've tried saving a few scenes with 3.2.2-rc1 in https://github.com/KOBUGE-Games/jetpaca and couldn't reproduce the issue.

Could you narrow it down to specific situations where the epsilon-esque skew gets added to the scene file?

@akien-mga Sure. Try this code, get the tool code to execute, save the scene and inspect the tscn file.

tool
extends Node2D

func _enter_tree() -> void:
    transform = Transform2D()

If it works on your end you should also see this reset arrow appear, indicating a non-default value.

[akien-mga]

Thanks, I can reproduce it too this way.

I'll revert the cherry-pick for 3.2.2, we can bring it back in 3.2.3 once a system has been figured out to prevent this inconsistency.

[aaronfranke]

Why are we saving the decomposed numbers instead of the raw transformation matrix values like we do for 3D? I think it would be more accurate to do the latter.

If we saved the transform values, it would avoid bugs like this and #29625 (item 1) since a 90 degree rotation would be saved as (0, -1, 1, 0) instead of 1.5708 (which is 90.0002105 degrees), and a 180 degree rotation would be saved as (-1, 0, 0, -1) instead of 3.14159 (which is 179.999847 degrees). Skewing would be saved as part of the matrix.

[groud]

@aaronfranke I believe this was originally done that way to make things more easy to read in VCS. It also kind of eases editing the text file directly, eventually for debugging purposes I suppose. Maybe it was not worth in 3D, as reading transforms and rotations in 3D is more complex ? I don't know...

But I agree storing the transform2d directly would solve the problem in a nice way, so I think it's a change to consider.

[akien-mga]

Why are we saving the decomposed numbers instead of the raw transformation matrix values like we do for 3D? I think it would be more accurate to do the latter.

It would solve the issue as far as serialization is concerned, but I think it would still have the bug that the computed skew property in the inspector may be non-0, and would thus appear with a revert button, no? (Even if it would be shown as 0 by default as only 3 significant digits are printed for the decimal value.)

[groud]

It would solve the issue as far as serialization is concerned, but I think it would still have the bug that the computed skew property in the inspector may be non-0, and would thus appear with a revert button, no? (Even if it would be shown as 0 by default as only 3 significant digits are printed for the decimal value.)

Ah yes, this is true. Maybe we should not display the revert icon when the value is both close to the default one AND not stored on disk directly ? As properties which are not serialized are usually computed from another property, it might make sense to ignore a small deviation from the default value.

[akien-mga]

Ah yes, this is true. Maybe we should not display the revert icon when the value is both close to the default one AND not stored on disk directly ?

That could work yeah, there could be a PROPERTY_USAGE_* hint for it.

[groud]

That could work yeah, there could be a PROPERTY_USAGE_* hint for it.

I was thinking about simply checking if PROPERTY_USAGE_STORAGE was not set.

[akien-mga]

Note that the 3D approach of saving only the transform also has drawbacks: #26173.
In that issue @reduz even considers applying the same kind of serialization used in 2D for 3D to solve it.

[aaronfranke]

Good point, there are still edge cases where computing the skew from the transform would result in a non-zero value. Perhaps the inspector should run Math::is_equal_approx(value, default_value) to decide whether or not to show the reset button.

[groud]

@akien-mga That being said, regarding the 3D problem, a simple solution might be to store the rotations or the scale as a complement to the transform. So that they could be used to choose whether we want a negative scale or a rotation (maybe a simple boolean could suffice too, I don't know). I believe a similar strategy could be applied to 2D too.