Open stefandollase opened 8 years ago
replace references to the RecognisedVersion enum entries by string constants
I suggest a class that's constructed from a string, that way you keep some of the type safety and the ability to go version.isNewerThan("1.9.1-pre2")
.
Also, when you say "string constants", do you mean the public static final string version1_0 = "1.0"
kind of constant, or the "1.0"
kind? Because a version enum or constants for known/parsable strings would also be good to avoid problems like the difference between version.isNewerThan("1.9.1-pre2")
and version.isNewerThan("1.9.1_pre2")
, such an enum wouldn't need to be updated except when Amidst needs to introduce new features keyed off a version not already in the enum.
This might be slow, because it has to calculate checksums for many classes. We would have to test the performance.
I'm not sure why you would want to do that. If the classes don't match the manifest then doesn't Java refuse to run it? AFAIK this is why mods have to remove the file. The manifest gives the real-names and a hash-digest, since java file names must match the class name and Java's security features means we can trust the hash-digest, so we could either look up the realnames of classes we need by scanning the manifest digests against a list of class digests from the JSON document, or we hash the entire manifest file and look up that hash in the JSON document to get the version, and use the version to lookup the list of realnames.
Edit: I now assume you were trying to make a system that doesn't need to fall back on the existing class recognition when the manifest is missing. I was on a different line of thought - thinking of a system that would be bulletproof for original versions, and at least as good as what we have now for modded versions.
(I'm supposed to be working at the moment, so I've kept it short)
All the class files in the Minecraft .jars have their "modified" timestamp set to what looks like the build time. If the magic string were to include the build time of the class file it's reflecting, it should be able to differentiate every version.
Might even be readable with JarFile.getEntry(...).getTime()
We just need to make sure that those timestamps are preserved by the tools in modding pipelines, i.e. if you unzip a jar, add some files then rezip it with common software, does the "modified" timestamp remain unchanged. It should be preserved, since moving the files around should only adjust the "created" or "accessed" timestamps, but perhaps there is some buggy software out there that's inexplicably popular. We should ignore the seconds field as some ancient filesystems aren't accurate to the second.
Since Amidst will know if the timestamp is missing or wrong (known magicstring + build time that's more than one known-version off = timestamp is wrong), this could easily be coupled with other magic-string improvements we come up with, for extra redundancy.
Edit: Just confirmed the build time is preserved in Optifine installs, and Windows Explorer also preserves it when performing extract/compress/insert operations in Windows.
We can also use build time to know if we're looking at one of the snapshots that wasn't archived by the launcher and so didn't make it to our version list (known magicstring + unlisted build time + build time less than next version, or greater than previous version).
When I spoke about String constants, I meant the public static final string version1_0 = "1.0"
kind of constant. You might be correct about the wrapper class for the String, but I thought of this as an implementation detail. The main point is that only the versions that the code refers to appear as a constant in the code. The list of all available versions as well as their order is provided by a JSON document.
the difference between
version.isNewerThan("1.9.1-pre2")
andversion.isNewerThan("1.9.1_pre2")
This will be avoided, if all versions that are mentioned in the code are stored in constants, e.g. version1.isNewerThan(version2)
.
version.isNewerThan("1.9.1-pre2")
Do we still need this kind of statement? I thought this would be handled entirely by the version feature mechanism?
The reason for updating the version list independently from Amidst is to display the correct version to the user, e.g. in the window title. I think it is also nicer for Amidst to know the exact version, so it can be sure about the chosen version features. This enables us to handle the UNKNOWN
version differently. Currently, we need to handle it like the most recent version, since an unknown Minecraft version is most likely a new snapshot.
This might be slow, because it has to calculate checksums for many classes. We would have to test the performance.
I'm not sure why you would want to do that. If the classes don't match the manifest then doesn't Java refuse to run it? AFAIK this is why mods have to remove the file.
The problem is again, modded Minecraft. As you said, it has to remove the manifest file. Thus, to be able to support modded Minecraft, we need to calculate the checksum by ourselves. At least in some cases.
so we could either look up the realnames of classes we need by scanning the manifest digests against a list of class digests from the JSON document
This is what I described under Alternative solution.
or we hash the entire manifest file and look up that hash in the JSON document to get the version, and use the version to lookup the list of realnames
That is a variation of solution 2 to problem 1.
Edit: I now assume you were trying to make a system that doesn't need to fall back on the existing class recognition when the manifest is missing. I was on a different line of thought - thinking of a system that would be bulletproof for original versions, and at least as good as what we have now for modded versions.
This assumption is correct. One of my goals is/was to reduce code complexity by removing the amidst.clazz
package. But while writing this I already kind of noticed that this will be very hard to archive, if not impossible, because at some point we need the recognition if we don't want to do it manually for each version.
I was on a different line of thought - thinking of a system that would be bulletproof for original versions, and at least as good as what we have now for modded versions.
I am not sure, why do you think it is more robust to pre-calculate the mapping than it is to do it when loading the Minecraft profile? Should this not be deterministic?
All the class files in the Minecraft .jars have their "modified" timestamp set to what looks like the build time.
I am really surprised about that, since that makes the build kind of not reproducible. For example, if you build the jar file multiple times without changing anything, the jar files will still be different, because they contain different timestamps. This is why I removed the timestamp from the Amidst test data zip files.
Edit: Just confirmed the build time is preserved in Optifine installs, and Windows Explorer also preserves it when performing extract/compress/insert operations in Windows.
My Linux tools (Nautilus + Archive Manager) also preserve the timestamp when changing the jar file contents.
I think the first decision we need to make is what exactly is the goal:
We can certainly achieve multiple of these goals, but some of them are conflicting. For example, maybe goal 2 and definitely goal 4 increase the code complexity (conflicting goal 1).
Also, the goals 3 and 4 are conflicting. Either we deterministically assign a Minecraft version to each Minecraft jar file, if necessary UNKNOWN
, or we try to guess a Minecraft version for each Minecraft jar file by using several heuristics.
My opinion on this is, that the performance boost might not be worth the additional development overhead. At the end of the day, Amidst is a tool that is only used very occasional by most of the users. Thus, I prefer a maintainable code base over a slight performance improvement (goal 1 over goal 2).
Next, I also prefer goal 3 over goal 4, because using too many heuristics will probably lead to weird and hard to reproduce outcomes/bugs. Of course, we should keep at least the level of recognition that we currently have, because it would be odd behavior for a user to update Amidst to a newer version just to notice that it does not work with a Minecraft version for which it worked previously.
Finally, I think we should prefer goal 1 over goal 4 whenever possible. As I said before, this should not drop the level of recognition under the current level.
My goal with updating the magic-string function was to have every known Mojang release reliably and uniquely identifiable (I don't know if that counts as goal 3 or 4). I'm happy to leave this deterministic - i.e. if a jar doesn't match anything known then it's considered UNKNOWN
. This magic-string issue is what I meant by wanting a system that is "bulletproof for original versions". Whether we want to pre-calculate mappings is a separate question.
To keep the door open for modded jars, it would advantageous if the magic-string function still works when a jar is modded and just reports the original Mojang version it was modded from. However this is a secondary goal for the magic-string and doesn't have to be perfect since with modded jars that's impossible.
With the pre-calculation of mappings, I was hoping for a speed boost, but I think you are right in that a minor speed increase for a minor tool for a computer game is not worth the development hours, let alone the lifetime maintenence cost the complexity will add to the project. And yes, the amidst.clazz
system cannot be retired, so we would end up with two systems for the same job.
I am really surprised about that, since that makes the build kind of not reproducible.
It's probably the date of the last file that was modified, but high-tech compilers have already killed reproducible builds. Multi-threaded multi-stage compliers that are avilable on many different CPU architectures mean deterministic compilation can't be assumed - even on the same CPU. The Java spec has no requirement for deterministic output, while the C# spec goes a step further and forbids it! The compiler inserts a GUID into the build to ensure nobody naively starts depending on it. This is a shame, the Snowden revelations illustrate it would be nice to have this kind of assurance (though NSA itself seems to prefer introducing flaws into specs or source, rather than poisoning binaries). I wish modern compilers would have a flag or something - even gcc isn't deterministic, but at least with gcc it can be if you know which features to turn off (AFAIK).
That was off topic, but a topic that interests me. (Someone should write an app that compares binaries and reports whether it can guarantee they are functionally identical, because often the differences are small - function order, GUIDs etc)
The reason for updating the version list independently from Amidst is to display the correct version to the user, e.g. in the window title. I think it is also nicer for Amidst to know the exact version, so it can be sure about the chosen version features. This enables us to handle the UNKNOWN version differently. Currently, we need to handle it like the most recent version, since an unknown Minecraft version is most likely a new snapshot.
Yeah, downloading up-to-date mappings of magic-string -> version is a good idea.
It's already downloading something similiar isn't it? So this wouldn't be changing much?
version.isNewerThan("1.9.1-pre2")
Do we still need this kind of statement? I thought this would be handled entirely by the version feature mechanism?
It should be possible to do everything in the version feature mechanism. I'll try to think with this mindset.
However, restructing systems to fit with the version feature mechanism usually takes me hours so it's something I leave until last when all the code is finished. Many changes need to be made to many classes to insert something new into the mechanism and it's sometimes not clear if a version check is going to become permanent (e.g. checking for b1.6), so a system where you can't just say "isNewerThan" while developing could be frustrating.
I might also end up solving this b1.6 problem by changing the ObfuscatedNameRCD from
public ObfuscatedNameRCD(String obfuscatedName) {
to
public ObfuscatedNameRCD(String obfuscatedName, Version jarVersion) {
That way the translator can go
.ifDetect()
.obfuscatedName('fd', Version.b1_6)
.or().obfuscatedName('ff', Version.b1_6_1)
.or().obfuscatedName('fg', Version.b1_6_2)
.or().obfuscatedName('hg', Version.b1_6_3)
.or().obfuscatedName('df', Version.b1_6_4)
.or().obfuscatedName('sd', Version.b1_6_5)
.or().obfuscatedName('gf', Version.b1_6_6)
.or().obfuscatedName('hs', Version.b1_7)
.or().obfuscatedName('er', Version.b1_7_1)
.or().obfuscatedName('nb', Version.b1_7_2)
.or().obfuscatedName('nb', Version.b1_7_3)
Unless you also want to include obfuscated names into the version feature mechanism? (it's not really a version feature) In which case I'll need you to move translator creation into the version feature mechanism because I'm unlikely to come up with the same system you would.
A lot of this discussion has been misunderstanding, or me getting too excitable, so I've skipped a lot of it.
I learned something new today: Compilers don't work deterministically. I always assumed they are deterministic. Thanks for the clarification!
I do agree with you that they should work deterministically to be able to check whether a given binary was produced from a given source code. This would be really nice for security checks as well as build automation.
I agree that this is an interesting topic. However, it might be difficult to write such a comparison application. If it is really only the order of functions, it might be possible. However, different compiler versions might apply different optimisations and then this becomes much harder.
But as you said, this is off-topic. Are you interested in continuing this discussion? If you are, I suggest to do this via skype or something similar. You can send me your skype id via mail.
When I talked about heuristics I referred to approaches like:
These might work somehow, but I don't consider them as reliable and precise, or too precise in the case of checksums.
The reason for updating the version list independently from Amidst is to display the correct version to the user, e.g. in the window title. I think it is also nicer for Amidst to know the exact version, so it can be sure about the chosen version features. This enables us to handle the UNKNOWN version differently. Currently, we need to handle it like the most recent version, since an unknown Minecraft version is most likely a new snapshot.
Yeah, downloading up-to-date mappings of magic-string -> version is a good idea.
It's already downloading something similiar isn't it? So this wouldn't be changing much?
Currently, we only download similar information from Mojang web services, but not from our self-hosted Github Pages. Also, it is surprisingly hard to decouple the current Amidst code from the RecognisedVersion enum. I tried this once before, but gave up for the moment. I still have this laying around as a feature branch, so I might continue to work on the existing branch, or simply create a new one if the existing one is too outdated.
I was not aware that the version feature mechanism is hard to use. I thought it would be pretty clear, because one does not have to follow some weird if-else branching. Do you have a concrete question about how it works? I will be happy to explain it. Can I change it somehow to make it easier to understand?
Indeed, I think the obfuscated class name is a good match for the version feature mechanism. However, I agree the name "version feature" does not fit this use case too well, but this only reveals that I was unable to come up with a better name for the mechanism :-P Do you have an idea for a better name?
Another solution came to my mind: We already have the code to download the Minecraft jar and json files for a specific Minecraft version from Mojang. We could extend this to use the new information provided by the new Mojang JSON API. This enables us to also download the libraries. We could then go ahead and use this code for Amidst itself, so it does not need to use existing Minecraft profiles and it also does not need to recognise Minecraft versions. It simply downloads everything it needs. This also means, Amidst does no longer require a Minecraft installation on the computer where it is executed.
The main reason why I don't like this idea is, that it leads to Amidst duplicating the .minecraft
directory, which consumes memory and causes maintenance overhead for each Amidst user.
However, I think the idea to rely on metadata from the Minecraft launcher is a good idea:
The more I think about this, the more I like the idea of merging the concepts of selected Minecraft version and recognised Minecraft version (solution 3 to problem 1).
I just looked in the FAQ what I wrote about why the separation exists:
We use the recognised version to determine which Amidst features should be activated, e.g. the used mineshaft algorithm. We do this, because it allows Amidst to be executed only from the jar file, without knowing the Minecraft version.
This is the only reason I was able to derive from the code. The separate concepts already existed in the old code. I only applied names to them, so they can easily be told apart.
I guess the historical reason why these are separate is, that in the early versions of Amidst, there was no proper Minecraft launcher and thus no .json
file. However nowadays, we have a .json
file for each Minecraft jar file. This .json
file contains metadata about the jar file.
Also, the statement in the FAQ is actually wrong: We cannot run Amidst only from the Minecraft jar file, but we always also require the .json
file that matches the jar file, see here and here. I guess this also means that this else
branch is wrong and should throw an exception instead.
The .json
file contains the id
property, e.g.
"id": "16w14a",
Next, as described here, modded Minecraft versions that can be loaded via the Minecraft launcher seem to inherit the metadata from its unmodded Minecraft version, e.g.
"inheritsFrom": "1.8.9",
To sum it all up: We already always require a metadata json file to be available for each used Minecraft version. This file either contains a vanilla version string directly or it inherits a metadata file that contains a vanilla version string. Thus, this is a reliable and simple solution for problem 1.
The version list json document that replaces the RecognisedVersion
enum really only has to contain a JSON Object that maps each Minecraft version to the first Amidst version that supports the given Minecraft version. Thus, it provides information about which version strings are valid, which Minecraft versions are supported by which Amidst versions and the order of the Minecraft versions. We should take care that this json document is as complete as possible. It should at least contain all versions from the Minecraft launcher, even if they are not supported by Amidst. This is because to identify a version as unsupported, we need to identify it as a Minecraft version in the first place. We might also have to add Minecraft versions to the json document, that are no longer available from the Minecraft launcher, but I am not sure about this.
Do you know how this works?: If a user installs a Minecraft version and uses it for some time, what does the Minecraft launcher do when Mojang decides to remove the Minecraft version from the launcher?
If this all works as expected, we can also go ahead and declare unidentified Minecraft versions as unsupported instead of assuming that all unidentified Minecraft versions are new snapshots. If a new snapshot is released by Mojang, we can check whether it works like the currently latest version that is supported by Amidst. If it does, we can simply add the new version string to the json document that is hosted via Github Pages. If the Amidst code needs to be adjusted to support the new Minecraft version, we would release a new version of Amidst and update the hosted json document. The newly mapped version string will be mapped to the new Amidst version, to prevent old Amidst versions from working with the new Minecraft version.
I propose the following course of action:
RecognisedVersion
enum into a json file.
Problem 3 was already identified as not being a problem. Thus, we omit the pre-calculated class mapping.
Do you think this is a good solution? Did I miss a case where this might break down? Do you know how the Minecraft launcher handles an existing installation of a Minecraft version that is no longer available for installation?
If this is a good solution, I will implement it in a separate feature branch as soon as I get the time to write code.
I don't know if this is something you've considered, but since 1.9 individual world files keep track of the last version they were played in, and Minecraft displays that information in the world selector (and uses it to give warnings about possible data loss etc).
Would it make sense for Amidst to use that information to decide what Minecraft version to apply when you open a world from a save file?
@sab39 Thanks for the hint. I am aware that this information is available in some save games and I tried to think of a way to use it, but it seems to not fit the current GUI concept very well. That being said, I don't think that this feature request fits to this issue: This issue is about how we interact with the Minecraft jar file. The requested feature is about how we use the version information in a save game. Thus, I think this topic deserves its own feature request. Can you create one?
I'd be happy to but I'm not quite sure what it should say. I guess I'll create one and leave it fairly vague for you to update with more detail as you see fit.
This issue contains the discussion about whether and how to adjust the interaction with the Minecraft jar file. Feel free to participate!
Current steps
a
orxy
. Of course, Mojang works with proper names in their source code. The obfuscated class names are automatically generated. This means that if Mojang introduces or removes classes, there is a good chance that all other obfuscated class names are changed. This is the problem that needs to be solved by this step: We have to somehow identify the classes that we need to use from the Minecraft jar file. Currently, we do this by defining a set of requirements that are only fulfilled by the used classes, see here. This linked class translator currently works for all supported Minecraft versions. I have to say, that I am really surprised that it works so consistent across all the version since Minecraft 1.0, but it works. This step translates the obfuscated class names, which change between the different Minecraft versions, to a symbolic class name, which is basically made up for Amidst. The same class in different Minecraft versions is always mapped to the same symbolic class name.LocalMinecraftInterface
to provide biome information to Amidst, see here and here. AMinecraftInterface
will always be used by exactly oneWorld
instance at the same time, via theBiomeDataOracle
. AWorld
object provides mechanisms to obtain information about everything that is displayed on a map, e.g. the Biome at a specific location, the Slime Chunks, the Villages, Strongholds, ...World
, based on the Minecraft version it uses. For example, we only enable The End for versions since Minecraft snapshot 15w31c. Currently, we map Minecraft version specific information via the version features mechanism, see here. By providing only the recognised Minecraft version, we receive a set of information associated to that Minecraft version, see here.You can find more information about the steps 2 and 3 here.
Problems
I shortly discussed problem 4 here.
Current state of the discussion
This discussion is still in progress. You can find the discussion up to this point in #78 and #145. In the following, I will not explicitly link these again. Here is a short summary. I also added a few arguments, especially pros and cons.
Regarding problem 1
Mojang now provides a JSON document for each Minecraft version that is available via a public web API. It contains the size and sha1 checksum of the Minecraft jar file. We can use this to map a Minecraft jar file to a specific Minecraft version.
A variation of this approach is to calculate an own checksum and/or provide an own list of checksums. However, the problem with modded Minecraft still exists.
Don't use a recognition mechanism at all. Instead, use the selected Minecraft version directly. This might work for unmodded Minecraft versions that are selected via the Amidst GUI.
A variation of this approach is to have a map from the selected Minecraft version to a recognised Minecraft version. This allows several different selected Minecraft versions to map to the same recognised Minecraft version, which might be useful for modded Minecraft. Thus, the recognised Minecraft version would still be the unique identifier for a Minecraft version, but it might have several aliases - the selected Minecraft versions. As a fallback, Amidst might ask the user to manually select the recognised Minecraft version.
Regarding problem 2
Regarding problem 3
We should be able to store the pre-calculated mapping as JSON. The Amidst jar file would contain the most recent version of this JSON document at the time of the Amidst release. Furthermore, we can provide an always up-to-date version of this JSON document via Github Pages, just like we do this with the update document, see here.
Class
objects will still take some time.The main reason why I liked the idea to hard-code the class mapping in a JSON document is because it removes complexity from Amidst. But after looking into this more closely, it would probably not remove any complexity. It might even add complexity: We would still need the current class recognition code to generate the JSON document. Even though the code might not be executed by Amidst itself, it would still be executed by the Amidst developers as a devtool. This means we cannot removed the code, but we still need to maintain it. Additionally, we would add all the code the create, read and use the hard coded mapping. Furthermore, Amidst itself would probably still need to execute the class recognition for Minecraft versions where a mapping is not available.
Thus, all in all this will probably add complexity for a questionable performance boost.
Regarding problem 4
Alternative solution
META-INF/MANIFEST.MF
of each jar file. Afterwards, we can determine the Minecraft version of the Minecraft jar file by mapping the matched class names and/or checksums to a specific Minecraft version.Implementation notes
Here are the tasks that I think should definitely be implemented:
RecognisedVersion
enum to a JSON documentRecognisedVersion
enum entries by string constantsSince this post is quite long, I will be especially careful to clearly mark any edits that I might apply.
Lets start the discussion. I am interested about your opinions :-)
Edit: added numbers to the different solutions, so they can easily be referred to.