oskarkk commented 2 years ago

I think that setting spawns is one of the most non-intuitive things in pvparena (as of 1.15.2) - many bad reviews are mentioning setting spawns, and people are asking about setting them all the time - so it's great that you're doing something with it. But looking at the doc in 2.0 branch, and testing the plugin build of that branch, I have some suggestions.

Here's a table from the 2.0 doc:

Command	Definition
/pa [arena] spawn set (teamName) [spawnName] (class)	Define a spawn for an arena
/pa [arena] spawn remove (teamName) [spawnName] (class)	Remove a spawn for an arena

And some examples:

/pa ctf spawn set red spawn - sets the red team's spawn of the arena "ctf"
/pa ctf spawn remove red spawn2 - removes the second red team's spawn of the arena "ctf"
/pa free spawn set spawnEAST - sets "spawnEAST" of the arena "free"
/pa ctf spawn set red spawn Pyro - sets spawn only for Pyro class of red team

1. Brackets

The first thing that's wrong with this is applicable to all commands in the docs. I'd say you're presenting the syntax of the commands wrongly - square brackets are usually used with things that are optional, not required. Also, normal parentheses are seldom used when presenting syntax of the commands - angle brackets (<, >) are used more. See docs of the other popular plugins:

Essentials: https://essinfo.xeya.me/commands.html
WorldGuard: https://worldguard.enginehub.org/en/latest/commands/
Multiverse: https://github.com/Multiverse/Multiverse-Core/wiki/Command-Reference (here curly brackets are used for required args, but still square brackets for optional args)
LuckPerms: https://luckperms.net/wiki/Command-Usage

And it's like that not only in Minecraft commands, but also in tools like git (see for example https://git-scm.com/docs/git-commit), or Linux shell tools like cp, ls etc. (https://man7.org/linux/man-pages/man1/cp.1.html).

So, I think that it would be much better if we used the syntax that is used in almost every other plugin doc, so the table above should look like:

Command	Definition
/pa [arena] spawn set [teamName] \ [class]	Define a spawn for an arena
/pa [arena] spawn remove [teamName] \ [class]	Remove a spawn for an arena

If you agree with this, I can edit every doc page of pvp arena in both master and 2.0 branches to use this style.

2. Order of arguments

I think that in a command like this the required argument should come before optional arguments. It may also make the code for parsing these commands simpler. So, I would rather recommend /pa [arena] spawn set <spawnName> [teamName] [class].

Think of tab completion. It would be better if after writing /pa myArena spawn set the plugin would propose you spawn, lounge, exit, spectator, not a mix of spawn types and teams like this:

I think this mix is confusing.

3. Spawn types

I think that the spawn name being also the spawn type is a bad design. It's not intuitive. I'm proposing:

/pa [arena] spawn set <spawnType> [teamName] [spawnName] [class]

Spawn name would be optional for some types of spawns - exit and spectator, and also for lounge if the arena is FFA. Examples of commands with the syntax I'm proposing:

/pa ctf spawn set spawn red 1
/pa ctf spawn remove spawn red 2
/pa free spawn set spawn east
/pa ctf spawn set spawn red 1 Pyro
/pa free spawn exit
/pa free spawn lounge

It would really be easier for new users and more intuitive. I think that parsing of these commands would also be easier.

4. Change type "spawn" to something else, for example "fight"

Imagine you're just downloaded the plugin and you're using it for the first time. Look at this command: /pa myArena spawn set spawn. It's confusing. Why there's spawn two times? spawn is a type of spawn?

I'm proposing changing that to something like fight. It would be logical:

lounge - the point where player would spawn before fighting
exit - the point where player would spawn after fighting
spectator - the point where player would spawn when spectating
fight - the point where player would spawn when fighting

Looking at the command /pa free spawn set fight we can guess that we're setting the spawn that will be used for fighting.

5. Configuration

Also, I'm proposing a big change in how spawns are saved in the configuration files. Spawns in arena config in 2.0 right now look like:

spawns:
  red_spawn: world,105,72,-341,-80.45086669921875,11.999911308288574
  red_spawn2: world,105,72,-340,-62.626708984375,25.499597549438477
  blue_spawn: world,110,72,-341,-80.45086669921875,11.999911308288574
  blue_spawn2: world,103,72,-340,-89.74935913085938,10.649736404418945
  blue_lounge: world,103,72,-338,-62.626708984375,25.499597549438477
  red_lounge: world,103,72,-338,-62.626708984375,25.499597549438477
  exit: world,104,72,-340,-62.626708984375,25.499597549438477
  spectator: world,105,72,-340,-62.626708984375,25.499597549438477

I'm proposing something hierarchical, like:

spawns:
  fight:
    red:
      '1':
        location: world,105,72,-341,-80.45086669921875,11.999911308288574
      '2':
        location: world,105,72,-340,-62.626708984375,25.499597549438477
    blue:
      '1':
        location: world,105,72,-341,-80.45086669921875,11.999911308288574
      '2':
        location: world,105,72,-340,-62.626708984375,25.499597549438477
  lounge:
    red:
      location: world,103,72,-338,-62.626708984375,25.499597549438477
    blue:
      location: world,103,72,-338,-62.626708984375,25.499597549438477
  exit:
    location: world,104,72,-340,-62.626708984375,25.499597549438477
  spectator:
    location: world,105,72,-340,-62.626708984375,25.499597549438477

With spawns for classes, it would look like:

spawns:
  fight:
    red:
      '1':
        location: world,105,72,-341,-80.45086669921875,11.999911308288574
        class: Pyro
      '2':
        location: world,105,72,-340,-62.626708984375,25.499597549438477
        class: Swordsman
    blue:
      '1':
        location: world,105,72,-341,-80.45086669921875,11.999911308288574
        class: Pyro
      '2':
        location: world,105,72,-340,-62.626708984375,25.499597549438477
        class: Swordsman

It would be a major change, but I think it's the right direction, that will lead to cleaner code, simple logic, easier maintenance. The properties of spawn would actually be a separate properties in the configuration, they wouldn't be extracted from strings. Also, think about the future. With configuration like this, you could sometime add new properties to each spawn. A very simple example:

spawns:
  fight:
    '1':
      location: world,105,72,-341,-80.45086669921875,11.999911308288574
      class: Pyro
      greeting: Welcome to spawn point for pyros!
      healthModification: -2
    '2':
      location: world,105,72,-340,-62.626708984375,25.499597549438477
      class: Swordsman
      greeting: Welcome to spawn point for swordsmen!
      healthModification: 2

In this example when spawning in each spawn the player would get a different welcome message and would spawn with specified change in HP. Of course that's only an example what could be done with configuration like that - it could be used by some new modules or something.

6. Alternatives: maybe spawns with unique names? Separate subcommands for setting the class, team, offset?

It's an alternative for what I described above in points 2, 3 and 5. Maybe it would be better if spawns had unique names within each type, then the configuration could look like:

spawns:
  fight:
    '1':
      location: world,105,72,-341,-80.45086669921875,11.999911308288574
      class: Pyro
      team: red
    '2':
      location: world,105,72,-340,-62.626708984375,25.499597549438477
      class: Swordsman
      team: red
    '3':
      location: world,105,72,-341,-80.45086669921875,11.999911308288574
      class: Pyro
      team: blue
    '4':
      location: world,105,72,-340,-62.626708984375,25.499597549438477
      class: Swordsman
      team: blue
  lounge:
    '1':
      location: world,103,72,-338,-62.626708984375,25.499597549438477
      team: red
    '2':
      location: world,103,72,-338,-62.626708984375,25.499597549438477
      team: blue

Then you could change the team of an existing spawn with a new subcommand like:

/pa [arena] spawn setteam <spawnType> <spawnName> <newTeam>

And then you could also set class with a separate subcommand:

/pa [arena] spawn setclass <spawnType> <spawnName> <newClass>

Then it would be easy to edit spawns of an arena - change the team of some spawn without setting and deleting, etc. Also, there's this in the doc:

/pa [arena] spawn [spawnname] offset X Y Z

I'm not sure if it's implemented yet, but it would go nicely with my proposals above, in the form:

/pa [arena] spawn offset <spawnType> <spawnName> <X> <Y> <Z>

And the config could look like:

spawns:
  fight:
    '1':
      location: world,105,72,-341,-80.45086669921875,11.999911308288574
      class: Pyro
      offset: 0,0.5,0
    '2':
      location: world,105,72,-340,-62.626708984375,25.499597549438477
      class: Swordsman
      offset: 0,0.5,0

Or maybe we could just have unique names for all spawns of the arena, which would be similar to what we have now, but the type still wouldn't be a part of the name. Config could look like:

spawns:
  '1':
    location: world,105,72,-341,-80.45086669921875,11.999911308288574
    team: red
    type: fight
  '2':
    location: world,105,72,-340,-62.626708984375,25.499597549438477
    team: blue
    type: fight
  '3':
    location: world,105,72,-341,-80.45086669921875,11.999911308288574
    team: red
    type: lounge
  '4':
    location: world,105,72,-340,-62.626708984375,25.499597549438477
    team: blue
    type: lounge
  '5':
    location: world,105,72,-340,-62.626708984375,25.499597549438477
    team: blue
    type: exit

Of course, user could have chosen any name for each of these spawns, it could be like:

spawns:
  'redteam':
    location: world,105,72,-341,-80.45086669921875,11.999911308288574
    team: red
    type: fight
  'blueteam':
    location: world,105,72,-340,-62.626708984375,25.499597549438477
    team: blue
    type: fight

... but the name of the spawn wouldn't be parsed as the type of the spawn. Theoretically you could have a spawn named "bluespawn" that would be a red team's spawn.

We could have commands like:

/pa [arena] spawn set <spawnName> [spawnType] [teamName]
/pa [arena] spawn setteam <spawnName> <teamName>
/pa [arena] spawn setclass <spawnName> <className>
/pa [arena] spawn settype <spawnName> <typeName>
/pa [arena] spawn remove <spawnName>
/pa [arena] spawn rename <oldName> <newName>
/pa [arena] spawn list [spawnType] [teamName]

Not sure if it would be better than just staying with what I described in points 3-5.

Pros:

it would be very versatile, you could easily do everything with each spawn with commands - change its type, team, rename it... (but with the solution from points 3-5 you could easily do the same thing by editing the config file)

Cons:

harder to understand for new users
it could be more chaotic (you would have to come up with an unique name for each spawn and could easily mix them up)

I would rather do this the way I described in points 3-5. Anyway, I would change the config files and separate the spawn types form spawn names.

The end

Every one of these points is a separate suggestion, every one of them can be implemented without implementing others. I really think that setting spawns is the most important part of the configuration of this plugin, so it should be done right - it has big impact on whether the user will start to use it, or delete it and forget about it. I also think that error messages in 1.15.2 are very bad, they don't really help average user understand what's wrong and how the command should be used - I myself was very confused when I started using pvparena. I haven't checked if this was changed in 2.0 - if yes, good, if not, I'll probably propose how they could be changed.

I can help with some of these suggestions, especially with the first point about square braces in docs - I can do this right now if you're agree with me.

Maybe I'll write an example doc page for the spawn command that will help you understand my proposal, and I think it would show how much clearer the spawn configuration would be if my proposal was implemented.

Thanks for your attention.

Eredrim commented 2 years ago

Hello,

Time for me to take time to answer you loooooooooooooong post 😋. I'll try to explain my personal point of view about all of this, I hope I'll not be too confusing. Anyway, let's start to review point after point!

1. Brackets

I totally agree with you about this topic. I chose to use brackets instead of angle brackets because it's really faster yo write (no need to escape characters), if you want to take time for changing this, feel free to do it. Anyway, you should let the arena name (first argument) as a mandatory word (so with angle brackets). I think it could initiate good habits to avoid issues while creation second (and more) arena.

2. Order of arguments

The choice I made for the order of arguments is to go in the direction of user logical, like if you wrote a sentence. For instance, I want to set the red lounge, I'll write /pa myArena spawn set red lounge. Seems more fluent for me 😇

3. Spawn type

We already are really near from this syntax. I'm afraid that creating a mega command with seven (or more) arguments may be counterproductive.

4. Spawn name

I'm totally share your opinion about this, it's completely ambiguous. I think we have to change either the name of the spawn type (from "spawn" to "fight") or the command name (for instance /pa tppoint set ... or /pa location set ...)

5. Configuration

Concerning that, it's clearly not my priority for the moment. Currently, the config file is directly edited by a very minority of users (for the spawns part) and even in this case, it's clearly human readable and editable. There would be no difference in terms of performance and maintainability. Yes code would be clearer, but that's my problem ^^

6. Alternative

For me this option is a bad idea, it will make more complex all commands for the player and the plugin is already enough hard-to-use.

Conclusion (mine one)

Many thanks for your feedback, it's really precious to have external point of views about PVPArena. That said, PVPArena is an old plugin, that was 10 years ago last summer. Oruss7 and me are the second generation of maintainers and we have tried to keep the plugin philosophy (large customization, modularity, separated context, etc) while improving the user experience and its stability. The 2.0 update objective is firstly technical, rewrite the code basis to remove duplicates but also make possible to implement new things without breaking all the plugin. The most of this aspect should be finished soon and, as of my side, I tried to remove a lot of things I thought too much weird. So yes, we still keep user experience and we'll continue to improve it. But I don't want to make a new refactor at this point of the work. I took that into consideration in my answers, in particular for points 5 and 6. Anyway you had here several good ideas and I clearly accept your help to change documentation syntax 😅 (it's really too time-consuming).

Thanks again for your help and have a nice day!

oskarkk commented 2 years ago

Hi, thanks for responding.

1. Brackets

Okay, so I'll do it. But I think that putting the arena name in angle brackets would be like lying - why show it as mandatory, when it really isn't? Documentation should reflect the behavior of the plugin, otherwise it'll confuse users - it'd be seen like the commands without arena arg are undocumented.

Also, I think that the way pvparena's commands are organized is bad, but that would be a completely separate issue. I'd move most of /pa commands to a separate command like /arena, and subcommands affecting the whole plugin would be in /pa, and subcommands affecting a single arena would be in /arena. It's would be like WorldGuard commands: /wg (reloading the plugin, info, global tools and something) and /rg (editing the regions), or Duels plugin commands: /duel (which you use to initiate a duel) and /duels (which you use to manage the plugin).

Anyway, if you want to discourage using /pa without arena name, it would be best to remove this ability altogether, at least for admin commands (which could be moved to another command as I explained above). Describing arena arg as mandatory would be simply untrue.

2. Order of arguments

I'm not sure if basing on the order of words in a sentence is the right direction, but I can twist your example :P You could think of a sentence like /pa myArena spawn set lounge (for the) red (team).

I generally think that putting optional arguments before mandatory arguments is a really bad practice. You don't usually see commands structured like that in other plugins, unless they're like flags in shell commands (WorldGuard has options in commands that look like /rg addowner -w <world> <player> - where the whole -w <world> is optional; CoreProtect has commands like /co rollback radius:5 time:8d where every arg after rollback is optional and order doesn't matter). I reeeaally think that putting optional <team> argument before mandatory <spawnname> will be the most confusing thing for users in this command. You realize that using /pa without arena name is confusing for users, and this is the same thing - optional arguments before mandatory. For an user who made FFA arena with /pa <arena> spawn set spawn it would be more natural if making spawns in a teams arena would look like /pa <arena> spawn set spawn red.

I'd say it's the most important topic of my post, so I'd appreciate if @Oruss7 would comment on the issue of the order of the arguments.

3. Spawn types

Yeah, the 2.0 way is better than 1.x, but I think it'll still be confusing for users. Many arguments in commands are not uncommon in other plugins, and if a command like /pa [arena] spawn setclass <spawnName> <spawnType><className> (described in 6.) would be implemented (that can be done without my proposed changes to configuration), then you'd have something like /pa [arena] spawn set <spawnType> [teamName] [spawnName], which is the same number of arguments as now in 2.0.

But I'd say that the change I'm proposing wouldn't really make the command more complex. Right now we have:

/pa [arena] spawn set [teamName] <spawnName> [class]

... but the spawn name usually has two parts: spawn type and the rest of the name:

(from doc)

For multi-spawn, you can set everything as name, as long as name start with the spawn type.

So, <spawnName> is really like <spawnType>[spawnName], and currently the logic of the entire command is:

/pa [arena] spawn set [teamName] <spawnType>[spawnName] [class]

And now you could say that this command already has 7 args masked as 6 args! And my proposal is like just adding a space to that syntax:

/pa [arena] spawn set [teamName] <spawnType> [spawnName] [class]

And then putting the optional args after the mandatory args:

/pa [arena] spawn set <spawnType> [teamName] [spawnName] [class]

So my proposal is logically the same as the current command. It really isn't more complex than what we have right now.

4. Changing "spawn" to "fight" or something

Great! I don't like tppoint, but location sounds good. Changing this would be a relatively simple change, so if you decide on one of these options, I could try to do it and make a PR sometime in the future. And maybe I could implement some "updater" that would automatically change spawn to fight in config, or spawns to locations.

5. Configuration

Yeah, I understand that it'd be a lot of work and there are more important things to do.

In the documentation for 2.0 there is that "spawn offset" described, and it is said that you can set that offset for every spawn. Is that implemented already? I tried using that in game, and it didn't work.

6. Alternative config

Okay, I wasn't really sure if that idea would be the right thing to do, but I was kinda just brainstorming some ideas :P

Conclusion

It's great that you're keeping this plugin alive. I love the idea of this crazy customization, modules and all that. I'm mainly looking at this plugin from an user perspective - I've started using it a year ago, and I've read many reviews on spigot and users' questions on discord - and I'm feeling that it would be a lot more popular if the UX was designed better. I think there should be more commands (at least one more) with separated "scopes" (like I described in my response to point 1 above, e.g. separate /arena command), because mega-command-for-everything /pa is really confusing. And the other thing that is really confusing for users is creating the spawns, so I think that the spawn command should really be done right, even if that means refactoring it again. I'll gladly help with this if I'll be able to (and if you'll agree with some of my proposals).

Also, I'm planning to port my work on global classes and /pa class to 2.0 and later make a new command for setting global classes and implement it on both 1.x and 2.0 (you've seen my recent message on discord). So, it would be nice if you and Oruss could push your changes to github more frequently, so that I could keep up with them while making these small fixes, and so I wouldn't have problems like #141.

And it would be nice if we could have a separate (private?) channel on discord for contributors (that would be like 4 people right now 😅), because I'll surely have some questions to ask about the plugin and the current state of development, that would be better asked in chat instead of an issue on github or something.

Eredrim commented 2 years ago

Hello,

I'll avoid to spent more time to write a novel in order to answer you because it would be useless and then because you're now able to access our dev channel on discord 😄

Anyway I just want to be clear: we both have a different vision of what should be the good direction for the future PVPArena but we both want to make it better. So, keep in mind it isn't a kind of debate where me and you should convince ourselves. You exposed me your arguments, I shared my opinion and then I'll need to think if I'm going to integrate it and how.

Eredrim commented 1 year ago

As discussed, several of your ideas have been implemented. You can test it in 2.0 dev preview

Eredrim / pvparena

2.0: spawn refactor and docs - my suggestions #142

1. Brackets

2. Order of arguments

3. Spawn types

4. Change type "spawn" to something else, for example "fight"

5. Configuration

6. Alternatives: maybe spawns with unique names? Separate subcommands for setting the class, team, offset?

The end

1. Brackets

2. Order of arguments

3. Spawn type

4. Spawn name

5. Configuration

6. Alternative

Conclusion (mine one)

1. Brackets

2. Order of arguments

3. Spawn types

4. Changing "spawn" to "fight" or something

5. Configuration

6. Alternative config

Conclusion