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.

Sign up for GitHub

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

Jump to bottom

Clean up version flags #1047

Merged

Grossley merged 19 commits into UnderminersTeam:master from Jacky720:master

Feb 23, 2023

Contributor

Jacky720 commented Jul 12, 2022

Description

This PR removes the UndertaleData.GMS2_2_2_302, UndertaleData.GMS2_3, UndertaleData.GMS2_3_1, GMS2_3_2, GMS2022_1, GMS2022_2, GM2022_3, and GM2022_5 booleans in favor of creating an UndertaleGeneralInfo.GMSVersions enum, as well as UndertaleGeneralInfo.GMS2Version using said enum. Closes GMS2 flag spam #1045.
Additionally, UndertaleData.IsVersionAtLeast() has been updated to accept these newer versions, and UndertaleData.SetGMS2Version() has been added to easily change the value of GMS2Version.
All existing references to the previous booleans have been changed to use the above functions, as well as several checks for reader.undertaleData.GeneralInfo.Major >= 2 being simplified to the existing UndertaleData.IsGameMaker2() function.
Finally, UndertaleGeneralInfo.ToString() has been updated to read from GMS2Version and thus display a more accurate version in the title bar, helping with Display proper GM version in the window title #929:

Caveats

Breaks any external references to the booleans that are not part of the project or that I missed.
The version displayed is inaccurate for GM2022.6, as well as versions between GMS2.0 and GMS2.2.2.302 due to the limited nature of the pre-existing flags and infrequency of changes to the data.win format. This may be fixed by adding special clarification (e.g. "GMS2.0-2.2.0") to the display.

Notes

N/A

Grossley closed this

Grossley reopened this

github-actions bot commented Jul 19, 2022 •

edited

Loading

Download the artifacts for this pull request here:

GUI:

CLI:

Contributor Author

Jacky720 commented Jul 28, 2022

@Miepee You said you had comments on this? Just want to bump it.

Miepee reviewed

View reviewed changes

Contributor

Miepee left a comment

Haven't checked the actual replacements yet, only the new approach.

UndertaleModLib/Models/UndertaleGeneralInfo.cs Outdated Show resolved Hide resolved

UndertaleModLib/Models/UndertaleGeneralInfo.cs Outdated Show resolved Hide resolved

UndertaleModLib/Models/UndertaleGeneralInfo.cs Outdated

-                      return DisplayName + " (GMS " + Major + "." + Minor + "." + Release + "." + Build + ", bytecode " + BytecodeVersion + ")";
+                      if (Major == 2)
+                          return DisplayName + " (" + Enum.GetName(typeof(GMSVersions), GMS2Version).Replace("_", ".") + ", bytecode " + BytecodeVersion + ")";
+                      else

Contributor

Miepee Jul 28, 2022

Unnecessary else.

Contributor Author

Jacky720 Sep 9, 2022

That's just my preference. Keeping all returns on the same level.

UndertaleModLib/UndertaleData.cs

+                      /// <param name="minor">The minor version.</param>
+                      /// <param name="release">The release version.</param>
+                      /// <param name="build">The build version.</param>
+                      public void SetGMS2Version(uint major, uint minor = 0, uint release = 0, uint build = 0)

Contributor

Miepee Jul 28, 2022

Should this this error in any way when invalid major is given, instead of just doing nothing silently?
Also, why is this done with a major/minor/release/build approach? I'd personally like if enums were still used for this, for safety, but I assume that created too much boilerplate?

Contributor Author

Jacky720 Jul 28, 2022

UndertaleGeneralInfo.GMSVersions.GMS2_2_2_302 qualifies as too much boilerplate, IMO.

Most invalid versions are actually just versions that don't have any distinguishing characteristics in the data file, but I see what you mean about major versions in particular. I can add a throw there.

Contributor

Miepee Jul 28, 2022

UndertaleGeneralInfo.GMSVersions.GMS2_2_2_302 qualifies as too much boilerplate, IMO.

AFAIK, you don't have give the full name like that, GMSVersions.GMS2_2_2_302 should be valid too, but may need some specific usings.

Contributor Author

Jacky720 Sep 9, 2022

Added error: "Attempted to set a version of GameMaker {major} using SetGMS2Version". Not going to resolve this in case it's still possible to get in those usings and make it pretty.

UndertaleModLib/UndertaleData.cs Outdated Show resolved Hide resolved

UndertaleModLib/UndertaleData.cs Outdated Show resolved Hide resolved

Contributor Author

Jacky720 commented Sep 8, 2022 •

edited

Loading

To-do:

Resolve all these suggestions.
Confirm that more recent commits haven't broken anything.
Realize I can move the reader.AllChunkNames.Contains("SEQN") check into GeneralInfo
and do away with reader.GMS2_3. (there are some more references)
Add 2022.8 support.
Say, are there any other new-ish chunks that can be used for version identification since 2.3? When was FEDS added? (added 2.3.6, FEDS, 2.2.1, TGIN, and 2022.8, FEAT)

Jacky720 force-pushed the master branch from d62c867 to 1e1d464 Compare

September 9, 2022 00:47

VladiStep requested changes

View reviewed changes

UndertaleModLib/UndertaleData.cs Outdated Show resolved Hide resolved

UndertaleModLib/UndertaleData.cs

    
            @@ -495,19 +468,24 @@ private bool TestGMS1Version(uint stableBuild, uint betaBuild, bool allowGMS2 =
          
                      /// <param name="release">The release version.</param>

                      /// <param name="build">The build version.</param>

                      /// <returns>Whether the version of the data file is the same or higher than a specified version.</returns>

                      public bool IsVersionAtLeast(uint major, uint minor, uint release, uint build)

                      public bool IsVersionAtLeast(uint major, uint minor = 0, uint release = 0, uint build = 0)

Member

VladiStep Feb 12, 2023

This method shouldn't be used within (de)compiler, because compared to the boolean check (GMS2_3), it's expensive.

UndertaleModLib/UndertaleData.cs Outdated Show resolved Hide resolved

UndertaleModLib/UndertaleData.cs

Comment on lines -297 to -300

-                      /// <summary>
-                      /// Whether the data file is from version GMS2.3
-                      /// </summary>
-                      public bool GMS2_3 = false;

Member

VladiStep Feb 12, 2023 •

edited

Loading

You should revert this field removal, and update the description so it's clear that only this field is kept because of performance.
(see my commentary at IsVersionAtLeast())

Jacky720 force-pushed the master branch from 68417df to 659190f Compare

February 14, 2023 01:28

Member

VladiStep commented Feb 14, 2023

I suggest instead of setting CompileContext.GMS2_3 on its creation, you should:

Make the variable static.
Set it in either GeneralInfo.Unserialize() or UndertaleChunkFORM.Unserialize().

VladiStep reviewed

View reviewed changes

UndertaleModLib/Models/UndertaleGeneralInfo.cs Outdated Show resolved Hide resolved

VladiStep reviewed

View reviewed changes

UndertaleModLib/UndertaleChunks.cs

                       internal override void UnserializeChunk(UndertaleReader reader)
                       {
-                          if (reader.undertaleData.GMS2_3)
+                          if (reader.undertaleData.IsVersionAtLeast(2, 3) && !reader.undertaleData.IsVersionAtLeast(2022, 6))

Member

VladiStep Feb 16, 2023

Is a 2022.6 check necessary?

Contributor Author

Jacky720 Feb 16, 2023

Yes. That code block is to set the version to 2022.6, so if the versions n has already been set higher it would be redundant.

VladiStep reviewed

View reviewed changes

UndertaleModLib/UndertaleChunks.cs

                       internal override void UnserializeChunk(UndertaleReader reader)
                       {
-                          if (reader.undertaleData.GeneralInfo?.BytecodeVersion >= 17)
+                          if (!reader.undertaleData.IsVersionAtLeast(2022, 2) && reader.undertaleData.GeneralInfo?.BytecodeVersion >= 17)

Member

VladiStep Feb 16, 2023

(the same question as above)

Member

VladiStep commented Feb 16, 2023

Everything seems fine.
Just make sure that there are no scripts that you've missed.

VladiStep approved these changes

View reviewed changes

Contributor Author

Jacky720 commented Feb 23, 2023

Rebasing to update the change from #1048...

Jacky720 and others added 14 commits

February 23, 2023 15:06


          Create enum. Will fix use of old values soon.

6020f6a


          Use IsVersionAtLeast

c0684d7


          Add version set function

185a511


          Use version set function

b93eb00


          Add new 2022.5 handling

216b205


          Remove bools from scripts

69058ff


          2022.6 handling

0d40d22


          Several changes for Miepee's review

4ea88ea


          Re-institute reader.GMS2_3

1369de5

because it's haunted


          Remove the enum, use regular version nums instead

5a307de

that just leaves merge conflicts


          [m] rm redundant variables

dd3b225


          catching up

86acb95

Ooh, the generic readers and writers are actually important... that makes sense.


          Exception type from VladiStep

8f79f09

Co-authored-by: VladiStep <vlad2001.21@mail.ru>


          (De)compiler optimization, new GMS2_3 bool

0929b2e

Jacky720 added 5 commits

February 23, 2023 15:10


          rm UndertaleReader.GMS2_3

8beef10


          rm small versions from display where irrelevant

81ce1fd


          2022.3+ is no longer Studio

de52a37


          rm foreach of deprecated version flag

76bbafc


          static GMS2_3 for optimization

86ddbaf

Jacky720 force-pushed the master branch from a818e0a to 86ddbaf Compare

February 23, 2023 20:11

Grossley merged commit 417dc35 into UnderminersTeam:master

VladiStep mentioned this pull request

GameObjectCopy.csx corrupts data.win. #1248

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet