Possible improvements

[davidrios]

Hello.

Thank you so much for this repo, it has been a great help to me, especially since I'm a complete beginner with Unity.

I have create a fork and made several changes, so I was wondering what's your level of interest on them, if maybe I should open a PR backporting some or all of the changes. These are the changes, please let me know which ones you would like if, any:

1. I'm using the built-in render pipeline, so I have adapted the materials for that. Maybe I could make it an option when importing to use URP or BiRP.
2. I discovered that it's simply impossible to have the import happen in a single pass using the AssetPostprocessor. In order to be able to successfully do stuff like doodad placement, every asset must already be imported, so in my version I have moved the post-processing logic to a separate step that the user executes after the initial importing is done. This also opens up the possibility for the user to try again if some assets have failed to import for some reason, and also this could be where the option of URP/BiRP is located.
3. When importing WMOs, I'm creating a single prefab where each doodad set is nested in its own game object, so it's possible to reuse the same one and simply enable/disable each desired doodad set. Then on importing maps and placing the WMO, I insert the prefab and just enable/disable the doodad sets used in that instance. This requires a change in wow.export though, which I have opened a PR for.
4. When creating every prefab, I'm nesting the stuff in a game object so it's easier to work with then later, in case one wants to customize something. ADT chunks for instance have hundreds of submeshes for the terrain sections, so it gets really difficult to work with them when it's all in the root of the object.
5. I'm not sure if this is a problem specific to BiRP, but when importing models that have two-sided materials, one of the sides is always transparent. I researched it a bit and found out that Unity doesn't support two-faced materials and the only options are to change the shader to disable backface culling or duplicating the mesh with inverted faces. I didn't want to change the default shader, so I have implemented the second option, and the importer does that automatically. This could also be an option when importing.
6. I'm creating all materials at the same place where the texture is located, I think it's a bit more organized than creating all materials in a single folder.
7. There are some models for which it's simply not possible to correctly set up materials using only the information in the obj/mtl files, so I ignore all materials when importing objs and use the information in the JSON to create them later. This also allows me to correctly set up some materials that have different versions with different blending modes. This is much like another PR that I opened in wow.export for the Blender add-on.
8. Fixed a bug when setting up the collision meshes where they were rotated by 180degs in the Y axis.
9. Changed the post-process queue to a concurrent queue instead of a simple list. The pre and post-processing are asynchronous and can happen at the same time, which caused issues because items where being added to the list while it was in the middle of an iteration.

These are personal stylistic choices that I understand are pretty hard to swallow, so I don't expected them to be accepted:

1. Prefer using var where possible. I'm completely new to C#, but from what I could see on the internet it looks like using var on most places is the currently accepted best practice. Coming from TypeScript and Python, where most types are automatically inferred, I find that a lot more convenient.
2. I think the code has too much indirection, which makes it hard to read. This also creates the problem of lots of small functions that need to take a lot of arguments because they pretty much need the whole context from where they are called in, so I have inlined most of the ones that don't need to be reused and I thought made the code more readable.

Thanks again for the awesome work! I would love to hear your opinion on these changes.

[briochie]

Hi @davidrios! Thank you for the kind words, I'm very glad that you are finding use for my plugin.

I'm busy at the moment so I can't review your message in detail, but I wanted to go ahead and leave a note acknowledging your request. Some of the extended features really speak to me, such as the WMO doodad sets. I think there is a valuable case for that. The Asset Postprocesser is also pretty slow, so the idea of delegating some of that work is a wonderful idea.

I will try to get back to you before next week. I appreciate your patience, and I'm glad you found success in forking the repo for your own use. 🙏

[briochie]

@davidrios I'm finally freed up a bit so I can address the options you raised. Here are my thoughts on each item:

1. I love the idea of supporting both BiRP and URP. Having the user be able to configure this, or have the plugin automatically detect this, is something I think would ve very valuable. The final version of this I have in my head would allow users to configure their own shaders for whatever pipeline they choose to use.

2. I agree with the asset postprocessor. I think I had good intentions with that design angle, but it is not ideal for such large imports. I'd like to make the postprocessor collect imported metadata, perhaps, and simply track metadata for objects. I am very much open to better ideas and implementations.

3. Nested WMO sets is a fantastic idea, I love it. When I started this project, I was struggling to make nested objects work in a way I like. I have since learned how this works, and have had better organizing in my head since. I especially want to set up WMOs to use rooms/render groups so we could potentially add portal rendering as an option in the future.

4. Same as above - perfect idea.

5. I do try to handle this already in the package, but it's not perfect. Certainly room for improvement. May need some companion work on the wow.export app to help us more properly map materials and their rendering flags properly.

6. I'm mixed on this one - I like the idea of materials being set in one folder, but I conceed that when a project ends up using a bunch of materials, it become unusable. Plus some materials need different rendering flags but share textures. I am open to this change.

7. Yes, very much understand this issue, as mentioned above.

8. Sweeeeeeet. This would be nice to fix. I think I want to reconsider the axies that we use to define north, or example. Ideally this could be configurable, but there may not be a need for that. I think I want to make it so that the positive Z axis in Unity represents north, and the positive X axis to be east. It's been a minute since I double checked what the package currently uses, but I know in the past I changed it and I can't really defend that choice I made.

9. Hell yes, I would love to have this change in the repo.

As for the suggested changes:

1. I have put a considerable amount of thought into your suggestion. I think if we want to use implied types in the package, it should be a package-wide change. Some cases (such as structs) may not be possible, and I think those are okay as exceptions.

2. I'd like some examples to better collect my thoughts on those changes, but I am always in favor of more readable code, so feel free to open a PR and I will look.

I'm so glad you found use for my package! Thank you for the thought-out response and recommendations.

[briochie]

Oh, I also meant to add - I need to do some work on the project's roadmap and priorities before I resume working on it in my spare time. I would very much appreciate any recommendations you have when reconsidering this. I have a handful of features (water meshes, texture atlas separation) that are half-developed on my local machine, and need a reset for my own head.

Making some of these core changes - such as BiRP support, and making the package have some sort of config, would be a good place to start.

[davidrios]

Took me a while to get back to it, but I just opened a mega PR with most of the stuff I had developed in my branch. No BiRP for now, but it'll be easy to add it later.