garbagemule / MobArena

MobArena plugin for Minecraft
GNU General Public License v3.0
196 stars 150 forks source link

About gamemode 3 for spectators #411

Open danielgarciagarcia opened 6 years ago

danielgarciagarcia commented 6 years ago

Summary

Background

Feature request

What does it do?: The players who have died can watch the game with gamemode 3 instead of beeing on the spectator region.

Does it need new or changed commands? What are they?:

Does this feature affect other parts of MobArena?:

jwflory commented 6 years ago

Hi @MetalGearDaner! Thanks for filing a new issue on MobArena.

@ct-martin mentioned the same idea in an old ticket, #213. I guess we need to figure out if the spectator area is still helpful or not. Forcing spectator mode, as I see it, makes the spectator areas pointless, since anyone can fly through walls.

Does that sound right to you too, @MetalGearDaner? Or do you still think a spectator area is useful even with gamemode 3?

ct-martin commented 6 years ago

Personally I think it's useful to prevent spectators from escaping the arena. A movement event would be useful for this.

On Dec 7, 2017 8:23 AM, "Justin W. Flory" notifications@github.com wrote:

Hi @MetalGearDaner https://github.com/metalgeardaner! Thanks for filing a new issue on MobArena.

@ct-martin https://github.com/ct-martin mentioned the same idea in an old ticket, #213 https://github.com/garbagemule/MobArena/issues/213. I guess we need to figure out if the spectator area is still helpful or not. Forcing spectator mode, as I see it, makes the spectator areas pointless, since anyone can fly through walls.

Does that sound right to you too, @MetalGearDaner https://github.com/metalgeardaner? Or do you still think a spectator area is useful even with gamemode 3?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/garbagemule/MobArena/issues/411#issuecomment-349965414, or mute the thread https://github.com/notifications/unsubscribe-auth/AYJ2H9gJLTeLER4brUn8G7Fcpud0raxgks5s9-bIgaJpZM4Q5fER .

garbagemule commented 6 years ago

This is an awesome idea and definitely worth exploring! :)

For the sake of backwards compatibility, of course, the spectator area is still useful. My guess is that most people would prefer the gamemode, but I don't know enough about it to conduct a proper risk analysis of completely removing the spectator area in favor of the gamemode. Are the players completely incapable of pulling mob aggro, interfering with other players, etc.? At first glance, being able to fly is probably the only real risk. What about chat?

To keep players from leaving the region, a periodic check will suffice. A small, focused loop over all spectators every 5 or 10 ticks is good enough (and allows for slower servers to "catch up" over the next ticks if the loop gets unwieldy). I have always been against PlayerMoveEvent - it spams so much that it's a better CPU load tester than Prime95.

danielgarciagarcia commented 6 years ago

That's the point guys. I personally think that the current spectator area may be useful, depending on how the arena is built and what kind of experience the designer of the arena wants for its players. But definately a gamemode 3 for spectators would be very "powerful" and enjoyable for the dead players that haven't survived on the arena :(.

I agree with @garbagemule about the PlayerMoveEvent, it spams a lot, and I've seen several plugins and servers having performance issues due to that event. The idea of the check every 5-10 ticks is more suitable I believe.

As far as I know, this gamemode is completely invisible to mobs and players when playing. You can fly through walls and "enter" inside a player's view, so you can see exactly what the player is doing and where is he looking at, as if you where him. The risk about flying out of the arena should be "easy" to fix with the check we talked about before. And about chat, it seems that the user in gamemode 3 has the same chat abilities than any normal user.

The best option I think is adding an option on the configuration so that the mobarena designer can choose between this two spectator systems (region and gamemode).

Thanks a lot for responding this fast, It's amazing to see the developers of this plugin so committed to make it even better :)

garbagemule commented 6 years ago

So I think that this is actually an excellent opportunity to revisit the setup process and its requirements. If we can implement well-defined, rock solid behavior with the spectator gamemode, it can just be the default configuration, if a spectator warp has not been set. And then there has to be a way to clear the spectator warp as well, of course.

@MetalGearDaner Thanks for taking the time to get involved in the project! :)

ct-martin commented 6 years ago

Is a warp isn't set, what about having the player get put where they died? Also, I checked, and spectator mode is 1.8 so in that regard there isn't much of a backward compatibility issue.

Chew commented 6 years ago

Mind if I chime in? Thanks.

I kinda like what Mineplex did with their spectating.

Here's my proposition. When you die, you stay in the arena (as if you didn't die), but you enter vanish mode, and can't interact with anything and nothing can interact with you, if the arena is configured, you can also fly. When you right-click on a user, you enter spectator mode for them, left clicking spectates a different user. Shift (sneaking) exits the spectator mode.

ct-martin commented 6 years ago

@Chewsterchew that's called spectator mode ;) It's an actual gamemode like creative, survival, and adventure

Chew commented 6 years ago

I mean, yeah. "You enter spectator mode for them"

ct-martin commented 6 years ago

@garbagemule another random thought to consider - catch and cancel the PlayerDeathEvent, display a message, and then put them as a spectator

ct-martin commented 6 years ago

@Chewsterchew I meant the whole deal is part of spectator mode - vanishing, flying, no interacting, noclip, etc.

jwflory commented 6 years ago

@MetalGearDaner said… Thanks a lot for responding this fast, It's amazing to see the developers of this plugin so committed to make it even better :)

Thank you too! Just to help give you a realistic expectation, we're in the process of what will be a months-long process to bring new life into this project. We can't promise a quick implementation, but this feedback is super helpful for us as we move forward with coming up with plans for new releases.

Haileykins commented 6 years ago

I'm actually currently working on just this! Instead of Gamemode 3 however (to prevent clipping and exiting the arena), I gave them invisibility and flight mode while in Spectator Mode, and also created a new method in monster managers that makes the mobs stop tracking you once you've died (since you re-spawn at your death point in the arena). Upon leaving your returned out of flight mode and your potion effects (Invisibility and Speed 1) are revoked. The way I have mine set up, is that if spectate-on-death is true, and this new feature, invisible-spectate is false, run the normal course for spectating, take them to their spectate location and call it good, if both are true, run the new code and do what was said above, if none are true, follow the original code and return them to the point of joining with their inventory back

jwflory commented 6 years ago

@Haileykins What you detailed makes logical sense to me. I like the direction you are taking. :+1:

I'm not too savvy on the codebase, and @garbagemule is most qualified to give meaningful feedback. If you are interested, I can assign this issue to you while you work on it. When you make a pull request, I can take my hand on reviewing it (or try to wrap @ct-martin in on it).

Let me know if you want me to assign this to you officially. :slightly_smiling_face:

Haileykins commented 6 years ago

Your welcome to assign it to me officially if you would like, I'm not fully knowledgeable on GItHub so I don't know if I can/how I would make a second totally separate open pull request at the same time as my current one is open, but once that one is open, I can wrap this project up with a few simple clicks to create a new PR.

garbagemule commented 6 years ago

The thing that makes gamemode 3 attractive is that it is natively supported. It's a vanilla mechanic that places the player in a state that isn't trivially replicated otherwise. It essentially boils down to low effort to implement, low maintenance cost, low risk of breakage, high functional reward, and high confidence in "bases covered" - everything you want in a feature implementation. Comparatively, a custom implementation is high effort to implement, medium to high maintenance cost, medium to high risk of breakage, equally high functional reward, and low confidence in "bases covered".

With "bases covered", I mean all of the logic involved in preventing players from interfering with the arena session in any way. The big problem is that it is necessary to either blacklist or whitelist behavior (or a combination), and regardless of the path chosen, changes in the Bukkit API can result in a previously well-behaved feature catastrophically failing after a single update (one example of this was the explosion event split). A natively supported feature typically covers it all.

As for clipping. Depending on the arena region and structure, clipping may or may not be an issue. I can see it being a potential issue if spectators reveal information that is otherwise impossible for the live players to obtain. This is a general problem with granting the spectators a superset of the abilities of the live players, so even flying falls into this category.

Assuming that clipping is not an issue, I see no reason to choose the custom implementation over gamemode 3. Assuming that it is an issue, I need solid arguments as to why that is, and why the downsides of the custom implementation have a smaller negative impact on the project than clipping does.

Haileykins commented 6 years ago

The issue I would like to propse regarding gamemode three is the current ability to exit the arena, clip through arena walls, and find possibly hidden architecture (Redstone, Harder To Find Chests, etc), that could impact gameplay by way of abusing the GameMode 3 System as an exploit rather than a feature, you can not natively remove clipping however that would be the most idea way to use the function in MobArena. The feature implementation I have proposed regarding this matter uses still vanilla Minecraft features and no deprecated code calls, and should be fluent in every way possible. Its also configurable and can be allowed or disallowed on a per arena basis. Instead of reiterating what the new feature does, considering I'm sure you've already read it, I will just remind you of simplicity. What has been done here is actually very similar to how HyPixel handles deaths in bedwards (aside from integration with MonsterManager of course), the formatted method of death sequence has become essentially universal in Minecraft MiniGame types. Hopefully I have made more sense here and laid out the reasons for the seemingly obtuse implementation.

And I would actually like to call attention to this post you made in this thread:

This is an awesome idea and definitely worth exploring! :)

For the sake of backwards compatibility, of course, the spectator area is still useful. My guess is that most people would prefer the gamemode, but I don't know enough about it to conduct a proper risk analysis of completely removing the spectator area in favor of the gamemode. Are the players completely incapable of pulling mob aggro, interfering with other players, etc.? At first glance, being able to fly is probably the only real risk. What about chat?

To keep players from leaving the region, a periodic check will suffice. A small, focused loop over all spectators every 5 or 10 ticks is good enough (and allows for slower servers to "catch up" over the next ticks if the loop gets unwieldy). I have always been against PlayerMoveEvent - it spams so much that it's a better CPU load tester than Prime95.

In the implementation I have proposed (pre PR), the spectator area, can and is still a thing, and can be used or not used depending on servers gameplay preference, if they have a colluseam type arena where theres no place they cant see the player, maybe a spectator area is the best option, they can use it, if they have a town map with buildings and such, invisibility and flight are added and monsters forget they exist and they can spectate the arena from a more personal approach.

In regards to removing the area in favor of spectator mode, I will say that Spectator type modes to view arena play after your death has become the par for most minigames that are new to the scene, it gives user more freedom and capability, however, I do think in this plugin, the physical area also has its pros and can really give a neat feel to colluseam type arenas, etc.

Mob Aggro/Interfering - In the proposed code I have, it actually talks to Monster Manager and calls a new method, Untarget, which tells all mobs targeting the player that just died to forget about them, and focus on a new target. I have not tested opening chests, modifying contents etc. but I think the fact that I make the call to mark them as a dead player and a spectating player addresses this, will need to test that issue.

Chat would not be affected, allowing dead players to talk within arena (or out of arena depending on how you have the arenas configured), though, being invisible also means they can't clip, therefore can't reveal hidden items to players.

Please forgive my lack of proofreading, doing multiple things at once.

slipcor commented 6 years ago

Bumping in because of notifications ^^

Running the risk of sounding like the grumpy old man telling you to "not become like him" - I very much was at the receiving end of this kind of breaking changes in my plugin PVP Arena.

If you design a self-written system that aims to emulate important logic like the existing spectator mode, you will run into issues the next time upstream changes things in a big way that causes your events to no longer fire the way you think they do.

Here is a small list of things off the top of my head, I probably forgot more. All of these were things that caused players to lose their inventory because they died spectating or generally they died inside an arena because I did not support it yet:

If I had had a spectator / adventure mode back then, I would have designed my plugin around this.

Best wishes to this plugin <3

Haileykins commented 6 years ago

@slipcor I appreciate your input however I'm a little confused by your intentions and meanings. I'm not trying to emulate spectator mode at all, this is a reverse-engineer of the way HyPixel runs their deaths in Bedwars. I have personally testing this continuously and (aside from the player laying down after respawn glitch that has been reported by many people) this implementation doesn't seem to have any negative affects on anything inventory or otherwise.

I may just be misreading your comment, if so I apologize, I'm in bed relaxing, its not a work my a$$ off day today so my brain is taking a rest too!

Everything I'm adding is functional as my local branch stands, tried and tested. However I neglected to test opening chests/interacting with renewable blocks, I know that players can harm the player or mobs, and that mobs will no longer target them.

If I have misunderstood anything please do let me know!

garbagemule commented 6 years ago

Okay, let's dial the "sales pitch" back a notch and get back on track. But to stay at a meta-level of discussion, I'd like to call attention to three concepts: maintainability, breaking changes, and iterative software development.


Maintainability is often forgotten about when working on exciting new things. Neglecting it, however, has the potential to make a massive, negative impact on the velocity of the project going forward. This is evident in several parts of the current code base.

It's important to note that a pull request is a one-off thing for the contributor, but it's a much bigger commitment for the maintainer. Every line of code that's introduced into the code base needs to be maintained by someone, and it is very rarely the contributor who ends up maintaining their contribution.

Maintainability of a code base or changeset can be measured fairly objectively with complexity metrics like lines of code, affected components, dependencies, etc. The more "stuff" you do, the more complex and less maintainable your work is. One line of code is objectively easier to maintain than hundreds of lines of code. You sacrifice maintainability when you "roll your own", so the value of the implementation being "your own" has to outweigh the heavy burden of losing maintainability.

The price for low maintainability is time and resources. This is also why "Hypixel does it" is a poor argument - we simply don't have the man power to do the grunt work required when you "live on the edge". We need to work smarter, not harder.


What slipcor is saying is what I also alluded to in my previous comment about "covering bases". You might think that you've got it all covered, but chances are you don't. And even if you do, you don't know what's going to happen down the line. You can't guard yourself against breaking changes, because you don't know what you don't know.

Breaking changes can happen both in whitelisting and blacklisting scenarios. The explosion event split that I mentioned in my previous comment was a blacklisting approach where "explosions" weren't allowed to damage arena structure. Spigot then introduced a second explosion event to cover "half" of the cases the first event covered, and the result was a lot of broken arena structures because updates had to be synchronized, but people didn't know. An example of a whitelisting approach gone to heck is the custom item parser in MobArena that's hopelessly obsolete by now.

The bottom line is that whatever bases you have covered right now in terms of mimicking a spectator mode are likely to be a subset of the bases you need to have covered later down the road. If you don't keep up, you'll eventually let bugs or exploits slip through, and the users eventually just stop using it (MobArena's soft-restore feature is an example of this). This ties directly back into maintainability.


Finally, iterative software development revolves around short, fast feedback loops where you discard all your assumptions about the users and their workflows and instead give them something to work with as quickly as you can, so you can harvest their feedback and use it the next development cycle. You often opt for something pragmatic, following the mantra that "done is better than perfect".

It's hard to argue that a custom implementation mimicking something that is supported natively is going to provide as much value as quickly. Not only are you effectively immune to breaking changes with the native approach, it's also a lot less code and thus way more maintainable. And if it turns out that the native approach fits the bill at the end of the day, that's a bunch of time saved that can be spent on the next development cycle or the next issue.


To sum up, I maintain (pun intended) that gamemode 3 is the better option for a first implementation of an enhanced spectator experience. The case against gamemode 3 is purely speculative (or at least based on very low quantity empirical evidence) is dwarfed by the immediate maintainability hit and the brittleness of a custom implementation.

slipcor commented 6 years ago

I have been editing this post back and forth and now the mule was faster than me. I needed to go long ago so I will quickly state some things I still have to add.

I have to say I missed your writings, garbagemule - I would look like I am copying your points so I will redo my answer :D

I have no doubts btw, what you have created probably works under best conditions and without intent to abuse it. Obviously if everyone plays by the rules and Spigot never changes/adds any events again, this is not an issue, but given what we know, players always find their way around the norm, and there will be additions to the gameplay that will require new events sooner or later, and if the plugin can prevent breakages in an easy native way, I would advocate for choosing the easier safer path that the API gives us, rather than further making the management of players a MobArena feature.

It's time for weekend. And now, I'll take a nap :)

Haileykins commented 6 years ago

My apologies, I'm still new to devving things I didn't write to begin with, I will take a look at ways to prevent noclip aspect of GM3

psyl0s commented 5 years ago

Not sure if I'm digging up a grave, but how's this idea going so far..?

garbagemule commented 5 years ago

There has been no development on this issue, but the spectator functionality is relevant to the Sessions rework.