Partly-Adequate / partly-adequate-mapvote

An addon for gmod which adds a comfortable way for voting on maps
https://steamcommunity.com/sharedfiles/filedetails/?id=2706252150
MIT License
7 stars 3 forks source link

Major improvements to PAM #23

Open geferon opened 5 months ago

geferon commented 5 months ago

I have started using this for a multi-gamemode server I have and it's great, with the exception that it's missing a few features or it needs a little more polish (mostly not on this repo, but the others, I will make PRs later).

For now the changes I've made are:

Reispfannenfresser commented 5 months ago

These are some nice improvements. You stumbled across some interesting problems. Thank you very much for bringing them to my attention and providing solutions.

There are a couple of things I'd like to see implemented differently however:

  1. The feature of allowing extensions to prevent themselves from being enabled is cool. I'd prefer it to use a new function (e.g.: CanEnable) though instead of hijacking Initialize for an additional purpose. Your implementation causes some issues at the moment, when an extension's enabled setting's value is changed. The extensions will still enable even though it doesn't want to be enabled.
  2. There is no proper way to access extension specific variables at the moment. Instead of using extension events, I'd prefer there to be a GetExtension function in the extension_handler. Extension variables can then be part of the extension's table and accessed globally. Instead of running PAM.extension_handler.RunReturningEvent("GetRoundLimit") you'd then be able to use PAM.extension_handler.GetExtension("custom_round_counter").round_limit_setting.GetActiveValue() to access the setting's value.
  3. It might be beneficial to load the gamemode maps pattern in OnInitialize instead of loading it every time a vote starts.
  4. I am unsure if I like supporting the "fretta_maps" option of the gamemode.txt. It is not listed in the wiki and seems like a deprecated relic. If a gamemode refuses to support the "maps"-option, it's the gamemode's fault. I have not yet found a gamemode that doesn't support the "maps"-option.

Thanks again for taking the time to make this PR. The populate_from_info setting is something I knew PAM was missing. It's very cool to see it implemented.

geferon commented 5 months ago

Sure! That sounds good, I wanted to hear your feedback as I was unsure about some of these changes, I will get to doing some changes and see what you think.

Also, to a few things you said:

  1. when an extension's enabled setting's value is changed. The extensions will still enable even though it doesn't want to be enabled.

I honestly forgot about that, I'll get to it, I'll also refactor extensions to have a CanEnable function, sounds better than the initialized hijack as said, although I wanted to ask, I noticed the OnEnable used to run before the Initialize (not OnInitialized) function, I changed the order but I didn't really look into it, is there some sort of reasoning behind this?

  1. I am unsure if I like supporting the "fretta_maps" option of the gamemode.txt

It is not listed in the wiki, but it's still being used by some gamemodes out there, especially those that were converted from the fretta age, such as:

These are relatively bad examples because they also have a "maps" entry (probably for legacy purposes and for mapvotes which don't account for the GM13 "maps" entry), but there are some out there, especially in the fretta archive one, which don't have a maps entry at all. I do think the "maps" entry should take precedence over the "fretta_maps" one, which I will change. If you don't want it I can remove it, but I would still add some sort of hook to be able to override the behaviour of the map selector, which brings me to my next point:

Extensions priority and hooks to disallow other extensions from loading. I think maybe having the ability of having some extensions loading before some others will be nice especially if you want to conditionally disable/not allow other extensions from loading, or so that the hooks of these extensions get called before the next ones. What do you think?

geferon commented 5 months ago

Oh also, should Initialize be called if it's disabled? I'd say yes, yeah?

geferon commented 5 months ago

There's a problem with using OnInitialize and that is that it doesn't get called when an extension is disabled, so even if it's re-enabled it won't run again. If I initialise the map list for the gamemode in the OnInitialize method, if this extension starts disabled and then gets enabled, this isn't going to be ran. Different solutions:

  1. Instead of OnInitialize use Initialize.
  2. New function, OnLoaded. Gets called either on initialized if enabled, or post initialized if it gets enabled afterwards, and it only gets called once, and when the extension is going to be used. I wanna personally go for the 2nd.
geferon commented 4 months ago

Any chance this can be looked at?

Reispfannenfresser commented 3 months ago

I am sorry for the large delay.

I think it's best, if you just upload your modified versions of the PAM addons to the workshop separately. I'll gladly update the workshop description of PAM to recommend your version for servers that want better gamemode voting support.

What you are doing is very cool, but I just don't have the time to deal with this at the moment. Sorry again.

geferon commented 3 months ago

That's fair, no worries!