Add logic for upgrade-relevant models #5110

CI09 · 2024-05-09T14:05:10Z

Brief Description of What This PR Does

Now upgrades can have "ScenePath": in organelles.json to have upgrade-specific models.

Related Issues

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

PR author has checked that this PR works as intended and doesn't
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
Initial code review passed (this and further items should not be checked by the PR author)
Functionality is confirmed working by another person (see above checklist link)
Final code review is passed and code conforms to the
styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

CI09 · 2024-05-09T14:20:22Z

Appearance tab has problems with updating though...

CI09 · 2024-05-10T11:03:39Z

It's ready for review.

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs

hhyyrylainen · 2024-05-13T06:12:27Z

src/microbe_stage/AvailableUpgrade.cs

@@ -35,6 +35,12 @@ public class AvailableUpgrade : IRegistryType
    [JsonProperty]
    public string IconPath { get; private set; } = string.Empty;

+    /// <summary>
+    ///   Optional model scene for upgrade


Suggested change

/// Optional model scene for upgrade

/// Optional model scene for upgrade which is used instead of the primary organelle graphics when this upgrade is applied

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs

hhyyrylainen · 2024-05-13T06:14:33Z

src/microbe_stage/editor/CellEditorComponent.cs

+        {
+            foreach (var availableUpgrade in definition.AvailableUpgrades)
+            {
+                if (availableUpgrade.Value.ModelPath != null)


String null comparisons should instead use string.IsNullOrEmpty.

hhyyrylainen · 2024-05-13T06:15:14Z

src/microbe_stage/systems/MicrobeVisualsSystem.cs

-                var visualsInstance = placedOrganelle.Definition.LoadedScene.Instantiate<Node3D>();
+                Node3D? visualsInstance = null;
+
+                if (placedOrganelle.Upgrades != null)


This is very similar code than in CellEditorComponent so I think this should be put into a helper method into the upgrades class. Or actually probably better to put into OrganelleDefinition as a method like GetEffectiveVisuals or named something similar.

hhyyrylainen · 2024-05-13T06:16:34Z

src/microbe_stage/systems/MicrobeVisualsSystem.cs

+                        {
+                            if (placedOrganelle.Upgrades.UnlockedFeatures.Contains(availableUpgrade.Key))
+                            {
+                                var upgradeRelevantScene = GD.Load<PackedScene>(availableUpgrade.Value.ModelPath);


These upgrade scenes should be loaded early in the same place organelle scenes in general are loaded (so added to OrganelleDefinition.Resolve).

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs

CI09 · 2024-05-13T11:00:21Z

I renamed ModelPath to ScenePath because model path now is model inside the scene.

hhyyrylainen · 2024-05-13T11:05:57Z

I think that is a good change to make this more uniformly named compared to how the organelle definition is setup.

If I had had futuresight when designing the organelle visuals system, I would have made a struct that holds the scene path, model path, and animation player path all in one as they are very closely related.

CI09 · 2024-05-14T20:01:28Z

I changed it now that upgrades' model/animation paths naming are the same as organelles' so it can be related easier (I hope it won't conflict)

Now, for example, to make slime jet visuals for pilus you have to add
"ScenePath": "res://assets/models/organelles/SlimeJet.tscn",
"DisplaySceneModelPath": "Armature/Skeleton3D/Cube",
"DisplaySceneAnimation": "AnimationPlayer"
to the upgrade in organelles.json.

What I am concerned about though, is the second commit (4e8c7d7), I don't know if it's good to do it the way like I did, but it worked for me and I am open for criticism.

hhyyrylainen · 2024-05-15T05:41:41Z

What I am concerned about though, is the second commit (4e8c7d7), I don't know if it's good to do it the way like I did, but it worked for me and I am open for criticism.

I don't know why you added that second part of that commit. It seems overtly complicated and I don't even understand why that code is necessary (this is a case where comments are really necessary to explain the reasoning behind the code).

hhyyrylainen · 2024-05-15T08:10:30Z

Actually on second read that code now always creates a new node visuals for an organelle, even when it already had visuals. This creates a bunch of nodes that are never attached to the scene tree causing a resource leak, not to mention the performance impact of re-creating a ton of unnecessary organelle visuals.

hhyyrylainen · 2024-05-17T12:03:29Z

This should now be easier to finish with this being merged: #5128

CI09 · 2024-05-17T16:00:59Z

It is now quite different (and simpler) from previous implementation thanks to #5128

CI09 · 2024-05-17T16:46:47Z

Could someone help with CI check failure though?

src/microbe_stage/AvailableUpgrade.cs

hhyyrylainen · 2024-05-20T06:29:09Z

src/microbe_stage/editor/CellEditorComponent.cs

@@ -1861,7 +1861,7 @@ private void RenderHighlightedOrganelle(int q, int r, int rotation, OrganelleDef
        bool showModel = !hadDuplicate;

        // Model
-        if (showModel && shownOrganelle.TryGetGraphicsScene(out var modelInfo))
+        if (showModel && shownOrganelle.TryGetGraphicsScene(null, out var modelInfo))


When moving an organelle, shouldn't this method take in a parameter specifying the organelle upgrades so that the visuals are consistent?

Upgrades are available from PlacedOrganelle and there is not much of it available for this line (shownOrganelle is OrganelleDefinition)

When moving an organelle, the variable MovingPlacedHex is of type OrganelleTemplate which contains upgrade data. That's why I made that comment. So updating this method to allow passing in upgrade data would hopefully make it relatively simple to pass those upgrades in when moving a hex.

hhyyrylainen · 2024-05-20T06:30:38Z

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs

-        // UpdateAlreadyPlacedVisuals();
+        microbeVisualizationOrganellePositionsAreDirty = true;
+
+        OnOrganellesChanged();

        UpdateStats();



The line for StartAutoEvoPrediction should be deleted as OnOrganellesChanged already calls that. This same thing is in the undo method.

src/microbe_stage/OrganelleDefinition.cs

hhyyrylainen · 2024-05-20T06:33:19Z

src/microbe_stage/OrganelleDefinition.cs

@@ -657,6 +657,30 @@ private Vector3 CalculateCenterOffset()
        return offset;
    }

+    private LoadedSceneWithModelInfo? TryGetGraphicsForUpgrade(OrganelleUpgrades? upgrades)


This has the boxing problem as well.

hhyyrylainen · 2024-05-20T06:33:35Z

src/microbe_stage/OrganelleDefinition.cs

@@ -657,6 +657,30 @@ private Vector3 CalculateCenterOffset()
        return offset;
    }

+    private LoadedSceneWithModelInfo? TryGetGraphicsForUpgrade(OrganelleUpgrades? upgrades)
+    {
+        if (upgrades != null)


I'd swap this the otherway around to early return if upgrades is null.

hhyyrylainen · 2024-05-20T06:37:14Z

src/microbe_stage/OrganelleDefinition.cs

+                var upgradeGraphics = availableUpgrade.Value.TryGetGraphicsScene();
+
+                if (upgradeGraphics != null)
+                {
+                    if (upgrades.UnlockedFeatures.Contains(availableUpgrade.Key))


I'd write this the other way around: check if an upgrade is present before doing the logic for fetching its scene.

hhyyrylainen · 2024-05-20T06:38:12Z

src/microbe_stage/OrganelleDefinition.cs

+                        if (availableUpgrade.Value.TryGetGraphicsScene().HasValue)
+                        {
+                            return availableUpgrade.Value.TryGetGraphicsScene();


Here, refactoring the TryGet method in the upgrade to directly return the data like TryGetGraphicsScene would allow simplifying this and removing the duplicate method call (or actually considering line 666 there's actually 3 calls to the same method here).

Now only the thing I commented on somewhere above and this (if it's necesary) yet to go.

…grade_logic

hhyyrylainen · 2024-05-21T05:58:57Z

src/microbe_stage/AvailableUpgrade.cs

 #pragma warning disable 169,649 // Used through reflection
+    /// <summary>
+    ///   A path to a scene to override organelle's display scene. If empty won't have a display model.


If empty won't have a display model.

Isn't this wrong or very annoying? Because upgrades that don't specify visual changes should just use the normal organelle graphics.

hhyyrylainen · 2024-05-21T06:00:05Z

src/microbe_stage/AvailableUpgrade.cs

+        }
+    }
+
+    public LoadedSceneWithModelInfo TryGetGraphicsScene()


Suggested change

public LoadedSceneWithModelInfo TryGetGraphicsScene()

public bool TryGetGraphicsScene(out LoadedSceneWithModelInfo overrideGraphics)

The signature should be like this to match the other TryGet methods I added, and also to follow the convention set by the C# standard library with its TryGet methods.

hhyyrylainen · 2024-05-21T09:28:03Z

src/microbe_stage/OrganelleDefinition.cs

@@ -275,10 +275,19 @@ public enum OrganelleGroup
    ///   The model info returned like this (as it may be a struct type this can't return a nullable reference without
    ///   boxing)
    /// </para>
+    /// <para cref="upgrades">


Suggested change

/// <para cref="upgrades">

/// <param cref="upgrades">

Typo in the XML tag.

When I do as you suggested, my IDE complains that it is lacking a name. I tried replacing cref= with name=, it stopped complaining but nothing really changed when I hovered my mouse above the function.

The right syntax is:

/// <param name="useDefaults">If true default checks are added automatically</param>

(just a random example I grabbed from Thrive).

CI09 · 2024-05-21T17:54:01Z

Might be good now.

src/microbe_stage/AvailableUpgrade.cs

hhyyrylainen · 2024-05-22T08:42:42Z

src/microbe_stage/OrganelleDefinition.cs

+    /// <param name="upgrades">
+    ///   Some upgrades alter organelle visuals
+    /// </param>


Suggested change

/// <param name="upgrades">

/// Some upgrades alter organelle visuals

/// </param>

/// <param name="upgrades">Some upgrades alter organelle visuals</param>

This fits on a single line.

hhyyrylainen · 2024-05-22T08:43:46Z

src/microbe_stage/OrganelleDefinition.cs

    {
-        // TODO: allow this to be affected by upgrades (add as a first parameter to this method)
+        var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out var upgradeGraphics);


Suggested change

var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out var upgradeGraphics);

var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out modelInfo);

This can directly write to the out variable of this method to save one data copy.

hhyyrylainen · 2024-05-22T08:44:50Z

src/microbe_stage/OrganelleDefinition.cs

-        // TODO: allow this to be affected by upgrades (add as a first parameter to this method)
+        var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out var upgradeGraphics);
+
+        if (hasUpgradeGraphics)


This doesn't need a separate bool so I'd recommend something like:

if (TryGetGraphicsForUpgrade(upgrades, out modelInfo) { return true; }

then also the line modelInfo = default(LoadedSceneWithModelInfo); should be removable to avoid one more data copy.

How removable? Do I put it in else?

No, just remove the line entirely, it should compile without it.

hhyyrylainen · 2024-05-22T08:45:32Z

src/microbe_stage/OrganelleDefinition.cs

    {
-        // TODO: allow this to be affected by upgrades (add as a first parameter to this method)
+        var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out var upgradeGraphics);


This part of this method should have the changes applied I mentioned above for the very similar block of code.

hhyyrylainen · 2024-05-22T08:46:40Z

src/microbe_stage/OrganelleDefinition.cs

+                bool hasUpgradeScene = availableUpgrade.Value.TryGetGraphicsScene(out var upgradeGraphicsScene);
+                if (hasUpgradeScene)
+                {
+                    upgradeScene = upgradeGraphicsScene;
+
+                    return true;
+                }


Suggested change

bool hasUpgradeScene = availableUpgrade.Value.TryGetGraphicsScene(out var upgradeGraphicsScene);

if (hasUpgradeScene)

{

upgradeScene = upgradeGraphicsScene;

return true;

}

if (availableUpgrade.Value.TryGetGraphicsScene(out upgradeScene))

{

return true;

}

hhyyrylainen

I think the architecture approach and other code is good now, I just want some improvements still to the struct data copy handling and removing some unnecessary intermediate bool values.

CI09 · 2024-05-22T15:35:46Z

If it's still to be tested, here's example object you can insert into upgrade in organelles.json:

"OverrideGraphics": {
"ScenePath": "res://assets/models/organelles/SlimeJet.tscn",
"ModelPath": "Armature/Skeleton3D/Cube",
"AnimationPlayerPath": "AnimationPlayer"
}

Edit: Corrected it for SlimeJet

hhyyrylainen

I did ever so slight changes and verified my last comments require no further actions. I didn't test the actual override graphics feature, but that can wait until we have a completed model and are adding it in a PR.

CI09 added the review label May 9, 2024

CI09 added this to the Release 0.6.7 milestone May 9, 2024

CI09 requested review from a team May 9, 2024 14:05

CI09 force-pushed the model_for_upgrade_logic branch 2 times, most recently from 1bfbcba to cc5ee47 Compare May 10, 2024 08:14

GameDungeon reviewed May 10, 2024

View reviewed changes

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs Show resolved Hide resolved

CI09 requested review from GameDungeon and hhyyrylainen May 12, 2024 20:22

hhyyrylainen requested changes May 13, 2024

View reviewed changes

hhyyrylainen added this to In progress in Thrive Planning via automation May 13, 2024

hhyyrylainen reviewed May 13, 2024

View reviewed changes

src/microbe_stage/editor/CellEditorComponent.Callbacks.cs Outdated Show resolved Hide resolved

CI09 requested a review from a team May 13, 2024 14:22

CI09 force-pushed the model_for_upgrade_logic branch from 08724fc to bb820d2 Compare May 14, 2024 20:15

Add upgrade-specific graphics for organelles

152b7eb

CI09 force-pushed the model_for_upgrade_logic branch from bb820d2 to 152b7eb Compare May 17, 2024 15:58

Style and warning fixes

e07646b

hhyyrylainen requested changes May 20, 2024

View reviewed changes

CI09 added 2 commits May 20, 2024 16:52

Logic and style fixes

ff5fd81

Revamp logic for getting graphics for upgrades

c10a09f

CI09 added 4 commits May 20, 2024 18:25

Switch to use null or empty check

a67dcad

Merge https://github.com/Revolutionary-Games/Thrive into model_for_up…

1bd20d0

…grade_logic

Make organelle move highlight be affected by upgrades

24788f8

Remove unused variable

0574713

hhyyrylainen reviewed May 21, 2024

View reviewed changes

Logic tweaks and fixes

8f9331f

CI09 force-pushed the model_for_upgrade_logic branch from bfbd337 to ae7bb13 Compare May 21, 2024 17:53

Small fixes

1a61474

CI09 force-pushed the model_for_upgrade_logic branch from ae7bb13 to 1a61474 Compare May 21, 2024 18:28

hhyyrylainen reviewed May 22, 2024

View reviewed changes

src/microbe_stage/AvailableUpgrade.cs Show resolved Hide resolved

hhyyrylainen reviewed May 22, 2024

View reviewed changes

hhyyrylainen requested changes May 22, 2024

View reviewed changes

Finishing touches

b9e8478

CI09 and others added 3 commits May 22, 2024 21:05

Remove redundant loaded scene casting

8164109

Merge branch 'refs/heads/master' into model_for_upgrade_logic

8b937f0

A few minor tweaks and comment clarifications

4b4f8f7

hhyyrylainen approved these changes May 23, 2024

View reviewed changes

hhyyrylainen merged commit 7bcd468 into master May 23, 2024
2 of 4 checks passed

Thrive Planning automation moved this from In progress to Done May 23, 2024

hhyyrylainen deleted the model_for_upgrade_logic branch May 23, 2024 08:42

	/// Optional model scene for upgrade
	/// Optional model scene for upgrade which is used instead of the primary organelle graphics when this upgrade is applied

	public LoadedSceneWithModelInfo TryGetGraphicsScene()
	public bool TryGetGraphicsScene(out LoadedSceneWithModelInfo overrideGraphics)

	var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out var upgradeGraphics);
	var hasUpgradeGraphics = TryGetGraphicsForUpgrade(upgrades, out modelInfo);

Add logic for upgrade-relevant models #5110

Add logic for upgrade-relevant models #5110

Conversation

CI09 commented May 9, 2024 • edited by hhyyrylainen

CI09 commented May 9, 2024

CI09 commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CI09 commented May 13, 2024 • edited

hhyyrylainen commented May 13, 2024

CI09 commented May 14, 2024

hhyyrylainen commented May 15, 2024

hhyyrylainen commented May 15, 2024

hhyyrylainen commented May 17, 2024

CI09 commented May 17, 2024

CI09 commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CI09 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhyyrylainen left a comment

Choose a reason for hiding this comment

CI09 commented May 22, 2024 • edited

hhyyrylainen left a comment

Choose a reason for hiding this comment

CI09 commented May 9, 2024 •

edited by hhyyrylainen

CI09 commented May 13, 2024 •

edited

CI09 commented May 22, 2024 •

edited