feat: update command line options for Terasology #646

keturn · 2021-07-28T03:29:07Z

Use posix-style command line option syntax for new (picocli-enabled) versions of Terasology.

Fixes #589.

How to test

The only Terasology command line option we pass is the Terasology home directory, so start games and make sure they're using the data directory specified in the Launcher settings.

Outstanding before merging

handle the mac-specific noSplash
detect Terasology version and behave appropriately
reinstalls game after starting launcher? feat: update command line options for Terasology #646 (comment)

keturn · 2021-07-28T05:13:15Z

oh, there is now another option we use, on Mac only:

TerasologyLauncher/src/main/java/org/terasology/launcher/ui/ApplicationController.java

Line 354 in 7bfe197

additionalGameParameters.add("-noSplash");

added by #613

I don't think the ApplicationController.startGameAction callback is the right place for this logic. I think what I'd propose is passing the GameRelease through instead of additional*Parameters that are not contained in Settings.

At the moment, GameStarter is easy to keep decoupled from things because its inputs are relatively simple data types.

On the one hand, GameStarter sounds like a good place to put the "figure out which command line arguments to use" logic.

On the other hand, constructing a GameRelease looks like it takes a modestly sized tree of nested objects, which I fear is a ticket to writing-unit-tests-gets-excessively-verbose town.

on the other other hand, the logic that determines isLwjgl3 isn't in GameRelease or ReleaseMetadata either, it's in a ReleaseRepository, which I think is questionable, as I'd expect this to have to be consistent across different Repositories?

keturn · 2021-07-29T17:00:27Z

picocli / POSIX-style command line

keturn · 2021-07-30T01:25:58Z

src/test/java/org/terasology/launcher/game/TestGameStarter.java

+        GameRelease release;
+        try {
+            release = new GameRelease(
+                    new GameIdentifier("5.1.0", Build.STABLE, Profile.OMEGA),
+                    new URL("https://repository.example"),
+                    new ReleaseMetadata(
+                            "# CHANGES",
+                            (new Calendar.Builder()).setDate(2021, 1, 1).build().getTime(),
+                            true)
+            );
+        } catch (MalformedURLException e) {
+            throw new RuntimeException(e);
+        }


😭 eight lines of imports and twelve lines here

🤷 as said in another comment: suggestions are welcome 🙃

src/main/java/org/terasology/launcher/game/GameStarter.java

skaldarnar · 2021-07-30T20:10:47Z

on the other other hand, the logic that determines isLwjgl3 isn't in GameRelease or ReleaseMetadata either, it's in a ReleaseRepository, which I think is questionable, as I'd expect this to have to be consistent across different Repositories?

The problem described in #646 (comment) is the main reason why I've put some logic into the ReleaseRepository as at that point we still know the "shape" of the version info...
As we don't have a comparator for GameIdentifier (or GameRelease) to determine "the version" after the fact, we bake some property right into the ReleaseMetadata. We could do the same for isPosix as we do for isLwjgl3.

I think the dumb pass-through proxy methods in GameRelease are the result of me attending some workshop on software best-practices and design patterns and then trying to apply (even the controversial parts) in real-world software 🙈

TerasologyLauncher/src/main/java/org/terasology/launcher/model/GameRelease.java

Lines 41 to 51 in 8d928d3

    
           public String getChangelog() { 
        
               return releaseMetadata.getChangelog(); 
        
           } 
        
           public Date getTimestamp() { 
        
               return releaseMetadata.getTimestamp(); 
        
           } 
        
           public boolean isLwjgl3() { 
        
               return releaseMetadata.isLwjgl3(); 
        
           }

I can totally understand your dislike for this solution now that I look at it again.

skaldarnar · 2021-07-30T20:18:51Z

On the other hand, constructing a GameRelease looks like it takes a modestly sized tree of nested objects, which I fear is a ticket to writing-unit-tests-gets-excessively-verbose town.

I tried to create a flexible yet simple model for what we are doing in the launcher. I tried to capture my intent in the respective Javadoc descriptions of the classes.

I'd like to keep the GameIdentifier a simple, immutable object with the goal to uniquely identify one of our game releases, ideally with support to compare versions to know how to sort them when displaying in the launcher. If we were only talking about a single profile this might just be a SemVer, if we were going to also launcher DestSol or different distributions of Terasology with the launcher this class would become more complicated.

The abstraction of the GameRelease contains the "physical representation" of a game identifier, e.g., where to get the artifact from, as well as other "release metadata". As described in #646 (comment) I'm not really happy with the metadata class, but on the other hand I didn't want to just dump all that stuff into GameRelease itself.

Feedback and suggestions are welcome!

skaldarnar

Overall I like these changes 👍

I'm not sure about the string comparison for the version, though - I think it would currently not work. I'd be happy to talk about game releases and versions again to figure out how we can get to a consistent versioning scheme now that we can also carry on looking forward.

keturn · 2021-07-30T21:46:03Z

I was complaining before I knew that GameIdentifier's non-string parameters are enums. It was still a lot to build, but it doesn't go as deep as I had feared.

and I expect the convenience thing could be addressed by some companion construction functions.

keturn · 2021-07-30T21:52:40Z

For use cases like this where it's after installation, we can also get information from the installation itself instead of the repository.

skaldarnar

Overall I'm good with these changes (I like the VersionHistory in particular).

I'd like to coordinate the merge of MovingBlocks/Terasology#4157 such that we try to minimize the number of broken preview builds in the launcher.

Depending on how exactly the SemVer lib compares versions, this might work out if the PicoCLI PR is one of the last ones merged before doing the release of 5.1.0.

However, I'd prefer the switch to POSIX-style arguments to be the first thing merged for the next upcoming release, i.e., 5.2.0-SNAPSHOT and adjust the version history entry accordingly. If that's too much effort, the approach mentioned above should be fine.

skaldarnar · 2021-08-07T22:02:27Z

src/main/java/org/terasology/launcher/game/VersionHistory.java

+     * See https://github.com/MovingBlocks/Terasology/releases/tag/v4.1.0-rc.1
+     */
+    LWJGL3("[4.1.0,)"),
+    PICOCLI("[5.1.0,)");


Also asked this on Discord: we've got a bunch of preview builds with engine-5.1.0-SNAPSHOT that don't use POSIX syntax yet - those would stop working, is that correct?

We just released https://github.com/MovingBlocks/Terasology/releases/tag/v5.1.0 without PicoCLI.

I'd merge the respective PR as one of the first things for 5.2.0-SNAPSHOT. I think the following IVY version matcher should work to include those snapshot builds:

Suggested change

PICOCLI("[5.1.0,)");

PICOCLI("]5.1.0,)");

I've updated PICOCLI to start at 5.2.0-SNAPSHOT.

src/main/java/org/terasology/launcher/game/VersionHistory.java

Changed the comparison mechanism from a `Requirement` to `Semver#isGreaterThanOrEqualTo` because Requirement doesn't parse pre-release versions.

… here

keturn · 2021-08-08T21:00:07Z

During playtest today, I successfully installed and ran stable release 5.1.1.

After restarting launcher, it did correctly use last-played-version, but it thought it needed to re-install:

Logs indicate that wasn't just a cosmetic thing, it did unzip the distro again when I hit the button.

I have not confirmed whether this bug is specific to this PR or not, but it certainly might be, given how much I messed with scanInstallations between this and #647.

skaldarnar · 2021-08-08T14:13:23Z

src/main/java/org/terasology/launcher/game/VersionHistory.java

+     * See https://github.com/MovingBlocks/Terasology/releases/tag/v4.1.0-rc.1
+     */
+    LWJGL3("[4.1.0,)"),
+    PICOCLI("[5.1.0,)");


We just released https://github.com/MovingBlocks/Terasology/releases/tag/v5.1.0 without PicoCLI.

I'd merge the respective PR as one of the first things for 5.2.0-SNAPSHOT. I think the following IVY version matcher should work to include those snapshot builds:

Suggested change

PICOCLI("[5.1.0,)");

PICOCLI("]5.1.0,)");

keturn · 2021-08-09T02:01:30Z

Shucks. I'm pretty sure that bug was introduced by #647. But that means this PR won't make it worse.

feat: update command line options for Terasology

7bfe197

keturn added 3 commits July 29, 2021 17:25

refactor(GameStarter): take GameRelease parameter

f08da26

refactor(GameStarter): put platform-specific launch parameters here

83528a9

feat(GameStarter): choose parameter syntax for GameRelease version

779a124

keturn commented Jul 30, 2021

View reviewed changes

keturn marked this pull request as ready for review July 30, 2021 01:29

skaldarnar reviewed Jul 30, 2021

View reviewed changes

keturn mentioned this pull request Aug 1, 2021

feat(GameIdentifier): include the engine version #647

Merged

keturn added 3 commits August 7, 2021 13:02

Merge remote-tracking branch 'origin/master' into feat/picocli

203abf3

fix(GameStarter): compare features against engineVersion

37f3411

chore(GameStarter): move lwjgl3 feature identification out of Repository

08cfda6

skaldarnar reviewed Aug 7, 2021

View reviewed changes

fix(VersionHistory): picocli begins with 5.2.0-SNAPSHOT

e92da9d

Changed the comparison mechanism from a `Requirement` to `Semver#isGreaterThanOrEqualTo` because Requirement doesn't parse pre-release versions.

keturn mentioned this pull request Aug 8, 2021

NPE in GameService.createTask [picocli branch] #649

Closed

fixup! refactor(GameStarter): put platform-specific launch parameters…

a528702

… here

skaldarnar approved these changes Aug 8, 2021

View reviewed changes

keturn merged commit 4327450 into master Aug 9, 2021

keturn deleted the feat/picocli branch August 9, 2021 02:11

keturn mentioned this pull request Aug 9, 2021

does not recognize stable releases as installed #651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update command line options for Terasology #646

feat: update command line options for Terasology #646

keturn commented Jul 28, 2021 •

edited

Loading

keturn commented Jul 28, 2021

keturn commented Jul 29, 2021

keturn Jul 30, 2021 •

edited

Loading

skaldarnar Jul 30, 2021

skaldarnar commented Jul 30, 2021 •

edited

Loading

skaldarnar commented Jul 30, 2021

skaldarnar left a comment

keturn commented Jul 30, 2021

keturn commented Jul 30, 2021

skaldarnar left a comment

skaldarnar Aug 7, 2021

skaldarnar Aug 8, 2021

keturn Aug 8, 2021

keturn commented Aug 8, 2021

skaldarnar Aug 8, 2021

keturn commented Aug 9, 2021

feat: update command line options for Terasology #646

feat: update command line options for Terasology #646

Conversation

keturn commented Jul 28, 2021 • edited Loading

How to test

Outstanding before merging

keturn commented Jul 28, 2021

keturn commented Jul 29, 2021

keturn Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

skaldarnar Jul 30, 2021

Choose a reason for hiding this comment

skaldarnar commented Jul 30, 2021 • edited Loading

skaldarnar commented Jul 30, 2021

skaldarnar left a comment

Choose a reason for hiding this comment

keturn commented Jul 30, 2021

keturn commented Jul 30, 2021

skaldarnar left a comment

Choose a reason for hiding this comment

skaldarnar Aug 7, 2021

Choose a reason for hiding this comment

skaldarnar Aug 8, 2021

Choose a reason for hiding this comment

keturn Aug 8, 2021

Choose a reason for hiding this comment

keturn commented Aug 8, 2021

skaldarnar Aug 8, 2021

Choose a reason for hiding this comment

keturn commented Aug 9, 2021

keturn commented Jul 28, 2021 •

edited

Loading

keturn Jul 30, 2021 •

edited

Loading

skaldarnar commented Jul 30, 2021 •

edited

Loading