dcronqvist / DotTiled

📚 A .NET library for loading Tiled maps and tilesets
https://dcronqvist.github.io/DotTiled
MIT License
21 stars 4 forks source link

Custom Class definitions shouldn't be required #42

Closed differenceclouds closed 1 day ago

differenceclouds commented 1 month ago

I get "System.Collections.Generic.KeyNotFoundException: 'The given key '(class name)' was not present in the dictionary.'" if I set the class property on anything in a map. I'm not sure what dictionary this is referring to, or how to add to it, and it's unclear how to proceed.

While I'm interested to learn the intended way to handle this, in my opinion, the "Class" property should be able to be used simply as a string that you can do whatever with, rather than something which requires a very strict implementation. You can type anything into a Class field after all, and using the Custom Types Editor isn't a requirement. It's just XML at the end of the day, and implementation should be flexible.

dcronqvist commented 1 month ago

If you want some more clarity on why these are required, or just want to learn the recommended way to define and handle these custom types, I recommend you check out this documentation on custom types. TL;DR: Whenever DotTiled encounters non-empty class/type strings, it will look up that corresponding defined type and do some stuff to fill in values in properties.

If you don't want to have to worry about this, or just want to write strings that don't correspond to defined custom types, you can do the following:

var classDefinition = new CustomClassDefinition
{
  Name = "YourClassName"
};
var loader = Loader.DefaultWith(customTypeDefinitions: [classDefinition]);

This will make it so the loader will find your class type and therefore not throw an exception - but you will not gain any of the power of custom types. If you have many of these classes, you can do something like:

string[] classes = [
  "MyClass1",
  "Monster",
  "Chest"
];
var classDefinitions = classes.Select(c => new CustomClassDefinition { Name = c });
var loader = Loader.DefaultWith(customTypeDefinitions: classDefinitions);

That should solve your issue. However, I do understand your argument that it should be possible to just have any arbitrary string. In the future, I may decide to add some kind of configuration option for allowing unresolved classes/types, but until then, this is the solution.

Please let me know if you still have issues getting it to work.

differenceclouds commented 1 month ago

Thanks for the reply! This workaround is fine with me. I'm hoping to translate a large project from another tiled library, and this will aid that process because I'm using the class property in my existing maps in a pretty loose, contextual way. I'm definitely going to try giving it a go the intended way though. I actually was only defining custom enums and bools etc in the types editor, and was using Templates in the way I could have been using custom classes, so it's probably good that I ran into this roadblock.

dcronqvist commented 1 month ago

Great!

I'll keep this issue open until we have a decision on how/when a potential configuration option for unresolved classes and types is implemented.

differenceclouds commented 1 month ago

One other thought: I think part of what bugged me is that this is the first instance I've encountered of the LoadMap function not working given any (valid) output from Tiled. So maybe if there are other cases where some (valid) output from a .tmx file will throw an exception on default LoadMap, that's something that should be looked at and considered.

TheBoneJarmer commented 1 month ago

I just encountered the same issue as well. Managed to load my TMX just fine in the past with TiledCS because I also followed OP's logic concerning the class name. It is just a random string that can be interpreted in any way by the user's game logic.

One other thought: I think part of what bugged me is that this is the first instance I've encountered of the LoadMap function not working given any (valid) output from Tiled. So maybe if there are other cases where some (valid) output from a .tmx file will throw an exception on default LoadMap, that's something that should be looked at and considered.

I agree with this. Imho you should not treat the class property like it is supposed to be matched to an actual class in C#. Admitted, the name class is a bit.. misplaced. You may not know this, but the class property didn't exist in older Tiled versions. In fact, if I recall correctly it was called type instead of class. It was introduced to give the object a bit more identity. Class is here actually a short form for classification and could be used to classify objects. May I suggest to just treat it like a normal string and just leave it at that? In my experience most people use it like this.

differenceclouds commented 4 weeks ago

It seems like there's no way to have a blank CustomEnumDefinition with just the Name field in the same way you've outlined for a CustomClassDefinition. Also, the FromEnum method defaults to expecting Integers instead of strings, which is different than the default for Tiled. I added a small fix for this in my PR, adding CustomEnumStorageType as a parameter to the FromEnum method.

dcronqvist commented 2 weeks ago

After quite a bit of deliberation, I understand that the fact that custom types are currently required to be defined can potentially cause a lot of confusion for many users of DotTiled. I therefore suggest that we make the custom types not required, and instead only attempt to find it during parsing to get correct default values etc.

However, this means that if a user makes use of custom types with default values, without setting the Resolve object types and properties option for exported maps, users will likely get a different map loaded with DotTiled than what they expect. All we can really do in this case is to provide a warning in the documentation about the behaviour - which isn't too bad.

dcronqvist commented 2 weeks ago

PR #57 contains an initial implementation for the above suggestion.

differenceclouds commented 1 week ago

Testing this out, and all seems well except for a detail with enums. The custom enums seem to only work with "Number" selected in the custom types editor (and an object created that uses the type), but I understand that's a separate issue.

dcronqvist commented 1 week ago

Testing this out, and all seems well except for a detail with enums. The custom enums seem to only work with "Number" selected in the custom types editor (and an object created that uses the type), but I understand that's a separate issue.

Interesting, would you be able to open up a separate issue for the problem you are facing? Please include some reproducible example that I can work with to do some debugging :)