Skip to content
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.

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

R2API.ContentManagement is the biggest codesmell I've contribued and needs rectification. #511

Open
Nebby1999 opened this issue Dec 25, 2023 · 8 comments
Labels
Code smell Issue describes ugly code or bad code practices

Comments

@Nebby1999
Copy link
Contributor

Nebby1999 commented Dec 25, 2023

(I hate my code)

In all seriousness, ContentManagement, while it was a good idea, needs to be refactored to be modified to use BepInPlugins instead of obtaining the assembly via the calling method.

Using the assembly causes issues as it can sometimes create unecesary content packs. Or overall unintended behaviour

This rectification should be done in a clean way ideally to avoid mods breaking, which could be difficult.

The other option of course, is to do nothing and completely axe ContentPack frameworks from R2API once Seekers of The Storm releases, and let modders properly use IContentPackProvider.

@harbingerofme
Copy link
Collaborator

How do you account for multiple bepinplugins in a single assembly?

@Nebby1999
Copy link
Contributor Author

Is there even a mod that does this?

@xiaoxiao921
Copy link
Member

There are some mods that do this though this is a non issue because of how PluginInfo work, it's just a member field tied to the BaseUnityPlugin instance so not a problem.

The problem with the current methods is that removing them would be breaking, so the best we can do is marking them as obsolete and provide overload that take a PluginInfo instead

@yekoc
Copy link
Contributor

yekoc commented Dec 25, 2023

The existing methods can be re-routed to the plugininfo by reflecting through the plugin attribute (finding the specific instance of BaseUnityPlugin isn't necessary because BepIn itself maintains a mapping between plugin id and info).
The issue regarding GetCallingAssembly pointing back to R2API during internal dispatch (which ContentManagement does have a bit more layers of then it needs) can be mitigated by climbing the stack frame,which isn't ideal,but would work fine for our purposes.
I can confirm that there are mods that bundle multiple plugins but it has been long enough since that was discussed that I don't remember any specific examples.

@Priscillalala
Copy link
Contributor

Is all the hassle to split content added by contentmanagement into separate content packs worthwhile?

@yekoc
Copy link
Contributor

yekoc commented Jan 4, 2024

There are some mods that count on it (primarily WAILA,though it's possible that there are other consumers that aren't advertising it)

@Priscillalala
Copy link
Contributor

Alright, here are my thoughts on content management then

  • R2APISerializableContentPack is a good replacement for SerializableContentPack and seems to work well, I haven't seen anyone have any major issues with it
  • ContentManagement exists for newer modders because experienced modders will be implementing their own content packs (or easily could). I dont think this is a bad thing because it lowers the barrier of entry to ror2 modding.
  • Given the above, ContentManagement should never aim to be a replacement for the normal content systems, but instead a stepping stone to them.
  • ContentManagement currently abstracts ContentPack itself, but I am not sure if this is necessary. ContentPack is actually quite straightforward to use - you create it and you add your content to the appropriate collection (which is essentially the same thing as using the ContentAddition methods). I would argue that the 'difficult' part of content management for an inexperienced modder would be implementing a content pack provider, not creating the content pack itself.
  • Given the above, maybe ContentManagement should only focus on providing content packs to the game - this could take the form of a method that takes an existing ContentPack and creates a generic provider for it internally, or a method that returns a new ContentPack for use that is already guaranteed to be provided to the game.
  • An additional benefit of this minimal API would be that modders would already be using a content pack and so could switch to their own content pack provider at any time

That is what I was thinking about regarding the external API. Internally, content management has its own problems:

  • Reflection has no place in an API like this
  • Content is being routed through the API in a weird way - it has to guess where a piece of content is supposed to go, despite it being added as an explicit type of content by content addition
  • it is using serializable content packs at runtime instead of content packs (though as mentioned before, perhaps it shouldnt be tracking content packs at all?)
  • API's like EliteAPI and ItemAPI have legacy dependencies on content addition that need to be addressed. These api's have useful utils that are annoyingly coupled to content creation: ItemAPI for example, requires me to define my item with ItemAPI if I want to use the item display utils

I have some interest in working on this content management rewrite; I am not sure how busy Nebby is or how much they wish to work on it though.

@Nebby1999
Copy link
Contributor Author

We can work it together if you'd like, currently i have my final defense on my thesis on the 11th, after that i'm officially on vacation.

@Nebby1999 Nebby1999 added the Code smell Issue describes ugly code or bad code practices label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code smell Issue describes ugly code or bad code practices
Projects
None yet
Development

No branches or pull requests

5 participants