Fix/build warnings 2024 09 15 #159

brmassa · 2024-09-16T18:21:36Z

Attacking #158

fixes for:

CS0108: 4
CS4014: 4
CS9191: 3
CS8619: 3
CS0649: 3
CS8765: 2
CS8767: 2
CS8620: 2
CS0659: 1
CS0661: 1
CS8714: 1
CS8607: 1
CS8509: 1
CS1998: 1

from 529 to 467 compilation warnings (not considering DotCast nor Veldrid)

NOTE: If one wants to focus on solving these issues, I suggest to adding
dotnet_diagnostic.<CODE>.severity = error
inside
.editorconfig.
Your IDE might immediately point to errors. It not, try compiling the code and check the log

brmassa · 2024-09-16T19:46:43Z

CS8629 and some other fixes

brmassa · 2024-09-16T20:36:36Z

CS8603 and some CS8601

down to 406 warnings

brmassa · 2024-09-17T00:33:34Z

down to 357 warnings

michaelsakharov · 2024-09-18T12:30:38Z

It might make more sense to disable the Readonly warning.
As there's a ton of cases with the Readonly's where it doesn't make a whole lot of sense.
Including a lot of public API intended for end users to change in runtime
Or ones that frequently change via Reflection using the Inspector in the editor

For example almost all the Components/Monobehaviour shouldn't have readonly fields, since the whole point of them are settings for the components.

brmassa · 2024-09-18T17:47:17Z

Understand the point. Some thoughts:

Dictionaries/Lists generally are not recreated but rather manipulated (add, remove items)
Fields, unlike Properties, are not meant to be public settable. f327527 only changes fields.
The static code analysis shows that any of these fields were changed inside the code. Of course, if there were meant to be used by the user, we could change them to properties.

In general, readonly allow the compiler to make A LOT of optimizations and also to raise aware when one tries to change them.

Paths forward:

Maintain the changes and revert case-by-case (when ones code depend on them)
Revert some classes
Revert all the commit

… async internally

… all possibilities

…l because it will be the index of a dictionary

…e dealt internally)

…used

…enhance code

…gnature

…d name conflict

…bset. just check if `subAsset` is not null

…t settings them nullable

…gned (typical for collections)

This reverts commit 1dd6a73.

michaelsakharov · 2024-09-19T01:43:01Z

The issue with switching to Properties is that moves away from the Unity API. Unity doesn't really even support properties-only fields, our serializer also doesn't handle them properly and it would be a reasonably big change for newcomers to c# who only know c# through Unity.

I think the read-only one makes sense to remove for the time being, What do you think @sinnwrig

PaperPrototype · 2024-09-19T02:18:24Z

hehe yeah im one of those unity C# only people :P

PaperPrototype · 2024-09-19T02:19:36Z

One thought I have is maybe not worrying about Unity only devs? We would only be perpetuating the problem.

…r reassigned (typical for collections)" This reverts commit aca944c.

brmassa · 2024-09-19T02:41:35Z

It's problem going to bite us in the future, but for the time being, I'm reverting it. There are still a lot (200+) other warning to attack.

michaelsakharov · 2024-09-19T02:42:02Z

One thought I have is maybe not worrying about Unity only devs? We would only be perpetuating the problem.

I mean a huge part of prowl is to be a unity alternative, so it's main audience by far will be unity devs. On top of the fact that it's not really a huge problem to perpetuate in the first place. Its less a problem and more a design choice in my opinion.

brmassa · 2024-09-19T02:52:21Z

ok. Commit reverted.

I believe Unity did some choices back in old days due lack of resources. Joachim (Unity ttech chief back then) repeatedly said there are several design mistakes that now they kinda have to live with.

Their serializer is sub-par. Sirenix (Odin) serializer is much more capable.

Using public mutable fields is a very bad practice. Basically all Static Analyzers will complain: Visual Studio, Rider, Codacy, Qodana, etc.

Sicne drag-and-drop replacement is virtually impossible, I believe we could have peace of mind on taking some things in other direction. As long it's well documented and explained to users.

sinnwrig · 2024-09-19T05:20:24Z

The issue with switching to Properties is that moves away from the Unity API. Unity doesn't really even support properties-only fields, our serializer also doesn't handle them properly and it would be a reasonably big change for newcomers to c# who only know c# through Unity.

I think the read-only one makes sense to remove for the time being, What do you think @sinnwrig

The readonly warnings should only be appearing because of C# style recommendations, correct? If so, it would probably be feasible to revert the warnings back to suggestions in the .editorconfig, since the compiler can't be aware of how a user and the serializer might interact with it. On top of that, the warning seems a bit like premature optimization, since the performance boost gained by setting fields to readonly is negligible in most cases unless it's in perf. critical code, in which case it can be optimized manually later down the line.

michaelsakharov · 2024-09-18T05:09:38Z

Prowl.Editor/Assets/AssetDatabase.Core.cs

@@ -168,7 +169,7 @@ public static void Update(bool doUnload = true, bool forceCacheUpdate = false)
                cacheModified = true;
                bool hasMeta = assetPathToMeta.TryGetValue(file, out var meta);

-                if (hasMeta)
+                if (hasMeta && meta is not null)


This should probably throw an error instead of continuing past as Meta being null here while hasMeta is true is pretty bad and should never occur.

michaelsakharov · 2024-09-18T05:13:50Z

Prowl.Editor/Assets/AssetDatabase.Core.cs

-            Debug.LogError($"No valid importer found for asset: {ToRelativePath(assetFile)}");
-            return false;
-        }
+        // TODO: FIXME: its always not null


If they modify the meta file itself for example breaking or removing the importer, then load it into the editor, the Meta file will successfully load however Importer will be null. So this check is required for that edge case. Not sure why the compiler says it cant be null.

michaelsakharov · 2024-09-18T05:15:05Z

Prowl.Editor/Assets/AssetDatabase.Core.cs

@@ -494,8 +496,11 @@ public static bool Reimport(FileInfo assetFile, bool disposeExisting = true)
        {
            var serializedAsset = SerializedAsset.FromSerializedAsset(serializedAssetPath.FullName);
            serializedAsset.Guid = assetGuid;
-            serializedAsset.Main.AssetID = assetGuid;
-            serializedAsset.Main.FileID = 0;
+            if (serializedAsset.Main is not null)


This should throw an error, Main not existing is an entirely broken asset, main should always exist.

michaelsakharov · 2024-09-18T11:02:02Z

Prowl.Editor/Editor/PackageManager.cs

@@ -170,7 +170,7 @@ private void DrawPackageList()
        }
    }

-    private void DrawPackageDetails()
+    private async void DrawPackageDetails()


I could be mistaken, But does making this method Async mean that the methods that call it continue onwards before this method finishes it's execution?
To my understanding, that's how Async methods work when executed from a synchronous caller. This would be problematic here, as it would change when/how this UI code executes, which could break the UI in unpredictable ways.

I believe the original intentional was calls to InstallPackage are not awaited and can execute at a later time asynchronous, but DrawPackageDetails in particular needs to execute and finish immediately on call for UI.

michaelsakharov · 2024-09-18T11:09:12Z

Prowl.Editor/Editor/PropertyDrawer/Drawers/Double_PropertyDrawer.cs

@@ -12,9 +12,9 @@ public class Double_PropertyDrawer : PropertyDrawer
    public override double MinWidth => EditorStylePrefs.Instance.ItemSize * 2;
    public override bool OnValueGUI(Gui gui, string ID, Type targetType, ref object? value)
    {
-        double val = (double)value;
+        double val = value is double v ? v : double.NaN;


NaN here might be a bit dangerous, just 0 or an exception would probably be ideal.
If this were to break and set things to NaN it would set users settings and values to NaN, which could spread and corrupt projects.

michaelsakharov · 2024-09-18T11:11:01Z

Prowl.Editor/Editor/PropertyDrawer/Drawers/PropertyDrawerEnumerable.cs

    protected override int GetCount(Array value) => value.Length;
-    protected override object GetElement(Array value, int index) => value.GetValue(index);
+    protected override object GetElement(Array value, int index) => value.GetValue(index)!;


This should probably be able to return null, so probably saver to make these methods nullabes. since its just an array GetValue for the inspector anything inside can be null which should be supported by the editor.

michaelsakharov · 2024-09-18T11:15:46Z

Prowl.Editor/EditorGuiManager.cs

@@ -308,7 +307,7 @@ public static void SaveScene()
            return;
        }

-        if (AssetDatabase.TryGetFile(scene.AssetID, out var file))
+        if (AssetDatabase.TryGetFile(scene.AssetID, out var file) && file is not null)


This should probably throw an exception if the file is Null, since it means the asset doesn't actually exist, yet its ID is in the database, this suggests a bigger problem with the Asset Database,

sinnwrig · 2024-09-19T05:29:00Z

ok. Commit reverted.

I believe Unity did some choices back in old days due lack of resources. Joachim (Unity ttech chief back then) repeatedly said there are several design mistakes that now they kinda have to live with.

Their serializer is sub-par. Sirenix (Odin) serializer is much more capable.

Using public mutable fields is a very bad practice. Basically all Static Analyzers will complain: Visual Studio, Rider, Codacy, Qodana, etc.

Sicne drag-and-drop replacement is virtually impossible, I believe we could have peace of mind on taking some things in other direction. As long it's well documented and explained to users.

On the public mutable fields, it seems a bit harsh to remove them completely because of bad design practice, since for many cases, having simple, raw, exposed values without a get/set (even if it's auto-implemented) is useful. If it's possible, it would eventually be nice to support [SerializeField] on property get/set. However, for most cases, something like [field:SerializeField] attribute is good enough to let the serializer get to an auto-implemented property.

In addition, while it might be good in the context of an organizational project, the engine should not be enforcing good practice or style on users, and the decision should ideally be the developer's to follow their own style guides and practices.

brmassa added 25 commits September 18, 2024 12:51

refactor: comment out private fields/variables that are not used

31eb773

refactor: comment out private fields that are not used

fa94257

fix: check of not null

09f0d3f

fix: replace async Task for void for a method that do not use any…

f8f2ebe

… async internally

fix: switch for a SceneViewPreferences.Instance.GridType must contain…

f9cdd59

… all possibilities

fix: public static int OrderlessHash<T> must ensure that T is not nul…

2277029

…l because it will be the index of a dictionary

refactor: optimizing Prowl.Runtime.GUI.Spacing to reuse code

90aef82

fix: assure that the t.Get is not null inside SerializedProperty.Merge

191e341

fix: KeyFrame's CompareTo and Equals methods allowing null (it will b…

aea007d

…e dealt internally)

fix: comment unused field and SceneMeshData.MeshData, that are never …

4497acf

…used

fix: new InspectorWindow.CreateInstanceCustomEditor to optimized and …

96f5777

…enhance code

fix: EngineObject.FindObjectsOfType should never return null

3a622ba

fix: some arguments passed using refinstead the in used in the si…

56e3ef1

…gnature

fix: use await to wait async method to finish

9ac5c96

fix: rename Prowl.Editor.SelectHandler's Equals to EqualsFunc to avoi…

194948f

…d name conflict

fix: using new and override to avoid conflict with inherited methods

0e6c93e

fix: unreachable code in Prowl.Runtime.PhysicsSetting

398b8e6

fix: variable is assigned but its value is never used

571d458

fix: check null before using it

3f7fb2c

refactor: in AssetsBrowserWindow.DrawFileEntry no need to use hasSu…

3adbc9b

…bset. just check if `subAsset` is not null

fix: CS8603 check for null before using it

d7a54f2

fix: several null checks

93617ed

fix: CS8605 checking possible null on casting

65d1ffb

fix: call Dispose() directly is obsolete. Using .Destroy()

6e6f8fe

fix: CS8625 assigning null to non-nullable fields. most fixes are jus…

8eb1d48

…t settings them nullable

brmassa added 5 commits September 18, 2024 12:56

fix: null checking

e302443

refactor: readonly on several fields/properties that are never reassi…

aca944c

…gned (typical for collections)

refactor: some extra fixes

792893f

Revert "fix: call Dispose() directly is obsolete. Using .Destroy()"

4c695f1

This reverts commit 1dd6a73.

fix: reverting replacement of Dispose() to Destroy()

100488f

brmassa force-pushed the fix/build-warnings-2024-09-15 branch from 3e2af96 to 100488f Compare September 18, 2024 18:35

Revert "refactor: readonly on several fields/properties that are neve…

0791469

…r reassigned (typical for collections)" This reverts commit aca944c.

michaelsakharov requested changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/build warnings 2024 09 15 #159

Fix/build warnings 2024 09 15 #159

brmassa commented Sep 16, 2024 •

edited

Loading

brmassa commented Sep 16, 2024

brmassa commented Sep 16, 2024

brmassa commented Sep 17, 2024 •

edited

Loading

michaelsakharov commented Sep 18, 2024 •

edited

Loading

brmassa commented Sep 18, 2024

michaelsakharov commented Sep 19, 2024

PaperPrototype commented Sep 19, 2024

PaperPrototype commented Sep 19, 2024

brmassa commented Sep 19, 2024

michaelsakharov commented Sep 19, 2024 •

edited

Loading

brmassa commented Sep 19, 2024 •

edited

Loading

sinnwrig commented Sep 19, 2024

michaelsakharov Sep 18, 2024 •

edited

Loading

michaelsakharov Sep 18, 2024

michaelsakharov Sep 18, 2024

michaelsakharov Sep 18, 2024

michaelsakharov Sep 18, 2024

michaelsakharov Sep 18, 2024

michaelsakharov Sep 18, 2024

sinnwrig commented Sep 19, 2024 •

edited

Loading

Fix/build warnings 2024 09 15 #159

Are you sure you want to change the base?

Fix/build warnings 2024 09 15 #159

Conversation

brmassa commented Sep 16, 2024 • edited Loading

brmassa commented Sep 16, 2024

brmassa commented Sep 16, 2024

brmassa commented Sep 17, 2024 • edited Loading

michaelsakharov commented Sep 18, 2024 • edited Loading

brmassa commented Sep 18, 2024

michaelsakharov commented Sep 19, 2024

PaperPrototype commented Sep 19, 2024

PaperPrototype commented Sep 19, 2024

brmassa commented Sep 19, 2024

michaelsakharov commented Sep 19, 2024 • edited Loading

brmassa commented Sep 19, 2024 • edited Loading

sinnwrig commented Sep 19, 2024

michaelsakharov Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

michaelsakharov Sep 18, 2024

Choose a reason for hiding this comment

sinnwrig commented Sep 19, 2024 • edited Loading

brmassa commented Sep 16, 2024 •

edited

Loading

brmassa commented Sep 17, 2024 •

edited

Loading

michaelsakharov commented Sep 18, 2024 •

edited

Loading

michaelsakharov commented Sep 19, 2024 •

edited

Loading

brmassa commented Sep 19, 2024 •

edited

Loading

michaelsakharov Sep 18, 2024 •

edited

Loading

sinnwrig commented Sep 19, 2024 •

edited

Loading