LazyDuchess / OpenTS2

Open source re-implementation of The Sims 2 in Unity
Mozilla Public License 2.0
248 stars 19 forks source link

Added the rest of the data in OBJD specification #18

Closed berylliumquestion closed 1 year ago

berylliumquestion commented 1 year ago

I was going to try to use reflection to alter a bunch of variables in a loop, but unfortunately boxing is an issue that can't really get around, so I gave up on that angle and decided to put all of it into one big dictionary. Let me know what you want me to change if you want to accept this PR.

LazyDuchess commented 1 year ago

I'm not a fan of the dictionary as I think it makes things convoluted and hard to read. IMO There's a lot of boilerplate and unclear code because of the way you formatted it with intertwined dictionaries, lists and tuples. Making them simple fields or properties would work a lot better I think.

I'm not sure why you wouldn't be able to use Reflection to loop over the fields/properties to serialize/deserialize? FreeSO keeps an array with the name of each property to be serialized in the order they are written to the file: FreeSO OBJD.cs

All properties are written as short integers so GUID becomes GUID1 and GUID2. The VM also only works with short integers so it works out.

berylliumquestion commented 1 year ago

As far as I know as soon you try to iterate through a list of reflected properties and assign them values you aren't going to be actually being changing the underlying values, but instead assigning a value to copy of a System Object which leaves the underlying variable unchanged, that's just how it's built. You'd need to unbox them, but to do that you'd need specify the variable in question, which defeats the purpose of reflection in this particular case. I believe it might be possible to do so with GetField and then using SetValueDirect, but I'll have to do some more reading on that.

Would you mind expanding on what makes the code hard to read? Is it not following some sort of convention? Is there too much of something? Too little? Are things in weird places? I know the list seems a bit odd, but that was the only way I could ensure the proper order since there are no generic dictionaries in c# which keep insertion order, they simply don't exist, at least in 8.0 as far as I know. SortedDictionaries are sorted, but not by insertion order unfortunately. Then again, I can just get rid of dictionary altogether since you've said you're not a fan of it so all of that would be a moot point if I got rid of it.

LazyDuchess commented 1 year ago

As far as I know SetValue already does unboxing and boxing, while SetValueDirect doesn't.

What I don't like is all the extra stuff put in place to check for the type of each property and the order of the properties. I think everything should be a short integer, that way you don't need a Tuple as the Value to hold the type and value itself separately, making access easier and removing all the nesting and conditions from the deserializing code which looks ugly.

You can use an OrderedDictionary - By default MS only provides a non-generic version of this in the System.Collections.Specialized package. but you can find a generic implementation here. So Instead of having multiple Lists and Dictionaries you can just have an OrderedDictionary with a string Key and an ushort Value for the data, and instead of using ".Add()" to initialize every value on the constructor you can initialize the complete Dictionary with every property set to a default value in the variable definition itself.

This would keep Dictionaries, but also avoids Reflection which is super slow and allows iterating over the Dictionary to write and read in a single loop while keeping the proper order of the values.

I guess you could also just use a normal Dictionary as by default the insertion order is kept unless an item is removed, but it's better to be safe I think.

berylliumquestion commented 1 year ago

See that's the problem with SetValue for GetProperty. It does the boxing automatically, but not in the way I originally thought it would. See here: https://codinghelmet.com/articles/value-type-property-setting I was shocked too that SetValue doesn't seem to do what it says it does. Plus, I just remembered that there's no reason to believe that reflecting preserves declaration order and in fact here's a Pull Request on the .NET platform to show that order is in fact not preserved, at least before 2020 https://github.com/dotnet/runtime/issues/46272 But given what was said in codinghelmet I might be able to still use reflection, just not to ensure the order. I'll try to make something with that, but it'll take me a few days at least.

If I turned them all into short type like you suggested and omit the 64 byte string in the beginning, I can turn the Dictionary into the format of a generic OrderedDictionary, if you're okay with using dotmore. It would take the form of something like this in ObjectAssetDefinition.cs: public OrderedDictionary<string, short> DeserializedData = new OrderedDictionary<string, short>() { {"GameVer_0", null}, etc... }

Then I can iterate through the items like this. foreach(KeyValuePair<string, short> element in temp) { reader.Seek(SeekOrigin.Begin, offset); asset.DeserializedData[element.Key] = reader.ReadInt16(); offset += sizeof(element.Value.GetType()); }

Unfortunately, I'd need to make a temporary copy of the dictionary because, if it's built like the other dictionaries, you can't iterate through a dictionary and then change anything in the dictionary within the iteration. Something to do with updating an internal "version number" as seen by the comment here: https://stackoverflow.com/questions/1070766/editing-dictionary-values-in-a-foreach-loop

berylliumquestion commented 1 year ago

Removed dictionary, but unfortunately had to keep SortedList.

LazyDuchess commented 1 year ago

You can just use a string array, I'm not sure why you're using a sortedlist when you decided not to go with the dictionary. Should be good to go after that. Definitely a lot cleaner than the first implementation.

berylliumquestion commented 1 year ago

Well, in fairness I could have used string array with dictionary since I'm going to assume arrays keep insertion order. Couldn't find any real definitive answers on whether retains insertion order or not, but it's now a string array as asked.

LazyDuchess commented 1 year ago

Cool, thanks! Arrays and Lists do keep insertion order. I suggest you also test things like this - if documentation fails then there's no better way to find out how things work.