-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update command line options for Terasology #646
Conversation
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
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 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 |
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭 eight lines of imports and twelve lines here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 as said in another comment: suggestions are welcome 🙃
The problem described in #646 (comment) is the main reason why I've put some logic into the I think the dumb pass-through proxy methods in TerasologyLauncher/src/main/java/org/terasology/launcher/model/GameRelease.java Lines 41 to 51 in 8d928d3
I can totally understand your dislike for this solution now that I look at it again. |
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 The abstraction of the Feedback and suggestions are welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
For use cases like this where it's after installation, we can also get information from the installation itself instead of the repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* See https://github.com/MovingBlocks/Terasology/releases/tag/v4.1.0-rc.1 | ||
*/ | ||
LWJGL3("[4.1.0,)"), | ||
PICOCLI("[5.1.0,)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
PICOCLI("[5.1.0,)"); | |
PICOCLI("]5.1.0,)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated PICOCLI to start at 5.2.0-SNAPSHOT.
Changed the comparison mechanism from a `Requirement` to `Semver#isGreaterThanOrEqualTo` because Requirement doesn't parse pre-release versions.
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 |
* See https://github.com/MovingBlocks/Terasology/releases/tag/v4.1.0-rc.1 | ||
*/ | ||
LWJGL3("[4.1.0,)"), | ||
PICOCLI("[5.1.0,)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
PICOCLI("[5.1.0,)"); | |
PICOCLI("]5.1.0,)"); |
Shucks. I'm pretty sure that bug was introduced by #647. But that means this PR won't make it worse. |
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
noSplash