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

Revert "[CSM] Add send_chat_message and run_server_chatcommand" #6091

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@rubenwardy
Copy link
Member

commented Jul 2, 2017

These functions have no good usecase, and are very abusable. The usecase of sending commands to the server should be done with mod communication channels, which the server needs to choose to use.

Original PR: #5747

This reverts commit 39f4a2f.

Related #5915

@rubenwardy rubenwardy changed the title Revert "[CSM] Add send_chat_message and run_server_chatcommand API fu… Revert "[CSM] Add send_chat_message and run_server_chatcommand" Jul 2, 2017

@rubenwardy rubenwardy force-pushed the rubenwardy:revert_csm_chat branch from 8d563a1 to bb238a8 Jul 2, 2017

@nerzhul

This comment has been minimized.

Copy link
Member

commented Jul 2, 2017

👎 see #5930 instead and help us to find good map soft limits permitting to finish flavours sent by servers
mods channels aren't here and we said this API can be modified at every moment and modders should verify it was modified, there is no problem to keep this hacky method waiting for mods channels

@rubenwardy rubenwardy force-pushed the rubenwardy:revert_csm_chat branch from bb238a8 to ecde37d Jul 2, 2017

@octacian

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2017

I understand why it is being suggested that this is removed, however, I'd far prefer if these APIs remained in place at least until mod communication channels are implemented as removal of run_server_chatcommand for one would result in the breakage of some very useful client mods that I use every day.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 3, 2017

#5930 has restrictions on chat messages, just in case you weren't aware.
But then, what are the uses of CSM chat messaging, is it really needed? So far the mods i have seen are ones that spam with automatic chat.
So far i'm neutral on this PR while i consider.

removal of run_server_chatcommand for one would result in the breakage of some very useful client mods that I use every day.

CSM is nowhere near finished, is missing restrictions and was released too soon, so we are free to remove stuff as it is unstable.

@red-001

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

Neutral on this.

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

List of mods using send_chat_message:

@bigfoot547

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

Instead of completely removing the function, make a no-op function and have it show a depricated message.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

List of mods using run_server_chatcommand:

Which is used by Client Side Protection which is actually definitively not evil.

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

y I forgot that one. But maybe we could just have server-side anti spam protection, ex. player is unable to speak if he repeats the same message twice in a minute?

@bigfoot547
Copy link
Contributor

left a comment

Please address

@@ -853,7 +847,7 @@ Please do not try to access the reference until the camera is initialized, other
* Returns with same syntax as above
* `get_fov()`
* Returns:

This comment has been minimized.

Copy link
@bigfoot547

bigfoot547 Jul 11, 2017

Contributor

Please remove this tab as it was a mistake

This comment has been minimized.

Copy link
@bigfoot547

bigfoot547 Jul 19, 2017

Contributor

@rubenwardy LINT is gonna be mad....

@@ -862,7 +856,7 @@ Please do not try to access the reference until the camera is initialized, other
actual = number
}
```

This comment has been minimized.

Copy link
@bigfoot547

bigfoot547 Jul 11, 2017

Contributor

And this one too

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

maybe we could just have server-side anti spam protection, ex. player is unable to speak if he repeats the same message twice in a minute?

Rate of chat messages is not the only issue, automated chat like the 'hi' mod is a problem also, that can't be protected against.
The flavours PR includes a way that servers can disable chat CSM, but i suspect all servers will just disable it. I can't see a need for CSM chat.

The usecase of sending commands to the server should be done with mod communication channels

CSM is unfinished so this can be done later.

👍

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@ExeterDad @shivajiva101 @Ezhh @tenplus1 @VanessaE what are your opinons as server owners?

@ExeterDad

This comment has been minimized.

Copy link

commented Jul 12, 2017

I would be happy with at least the option to disallow.
Currently I have to do hacky stuff to knock out the "Hi" mod. But it's only a short lived relief I'm sure.
@paramat Thanks for asking 👍
The Hi mod isn't really a big deal in the grand scheme of things. It only checks for the text related to joining. But any fool can code a bot that interacts with God only knows what and tear the place apart. Think IRC

@VanessaE

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

@paramat definitely revert/remove such functions.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Note that the flavours PR will not be able to restrict old clients, so restrictions will only be enforceable if you ban old clients, this will of course be forced by 0.5 where everyone has to upgrade.

@VanessaE

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

Also I would have to say these various CSM restrictions/removals should not be limited to 0.5.x - backport to 0.4.x if needed, and do a protocol bump, so those of us who want to avoid 0.5.x initially (which will be most server owners, I think), will be able to block abusive clients more easily.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

^ I agree. I think that is intended already.

@VanessaE

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

Just making sure. 😄

@Ezhh

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

To keep it simple - what Vanessa said.

@shivajiva101

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

I totally agree with Vanessa's assessment 👍

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 13, 2017

From now on we should only add CSM functions if it is a good idea, not add everthing automatically just because it can be, which seems to have been happening.

Any more dev opinions before a merge?

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

bdac127
And fixed some whitespace errors.

@mark-otaris

This comment has been minimized.

Copy link

commented Jul 15, 2017

@paramat

Did this have two approvals from core developers? This pull request is controversial. If it didn't have two approvals, I would like for it to be reverted.

If it did have two approvals, perhaps I should have commented sooner, but I would like for it to be reconsidered and reverted anyway. Mod communication channels do not exist yet and, more importantly, would only be useful for client mods provided by the server. There are many use cases in client-provided mods for send_chat_message and especially run_server_chatcommand. Some of them:

  • The waypoints mod uses run_server_chatcommand to teleport the player to a waypoint and to run commands with the position of a waypoint as argument.
  • The areas mod, used on many servers, is used entirely with server commands. Client-side mods to make setting areas more convenient, to display information about them or to modify multiple areas at once need to be able to send commands. I wrote one such mod with two client commands: one that protects multiple areas vertically, with their maximum size, and another that renames all areas connected to the one at my player's position.
  • On servers with the sethome mod, run_server_chatcommand could be used by a client-side mod to set a home periodically or to automatically set a home before dying.
  • The send_chat_message function is the only convenient way to use color in chat messages.

Furthermore, many players may want to create personal aliases or command helpers that are player-specific or server-specific and that would not be published on the Minetest Forums. I do this on IRC. It can be useful in any place that allows chat. These two functions could be used for warning and banning players, scheduling notices (with minetest.after, for events or other purposes), setting the time with a loop to keep it day while building.

The possibility of abusing send_chat_message and run_server_chatcommand (some server owners might even not like some of the use cases I gave as examples) is not an issue if they can be disabled with flavour limits.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

reverted. Controversial this should not be merged without flavour or acceptable solution.

@SmallJoker SmallJoker reopened this Jul 15, 2017

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

@rubenwardy
This has 2 core dev approvals and 1 core dev disapproving, so was mergeable, it has been open 13 days for dev opinions, 4 days ago i added my approval then waited 4 days and asked for more dev opinions, red-001 is neutral about it, and 4 server owners agree with it (many who disagree are those who have somewhat hacker-type attitudes).

Mark-otaris it has 2 core dev approvals, which makes it mergeable. It has been agreed that server commands are useful but should be done with mod channels, and they will be, you will not lose that feature.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Since flavours is ready lets merge that first, obviously, then this PR will need to be rebased, by that time hopefully it will have more opinions.

@mark-otaris

This comment has been minimized.

Copy link

commented Jul 19, 2017

Mod channels aren't useful for client-provided mods, which all the use cases I gave as examples were about. I think the use cases are useful and valid. Possible abuse issues are solved by #5930, which was merged five hours ago. There is reason to keep the send_chat_message and run_server_chatcommand functions and no reason to remove them.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

mods channels are useful for clients, but you just don't know how to use it atm :) in world of warcraft inter player mod communication uses a such mechanism, it permits for example if a player see an event by a trigger (for example a mob spawn) the channel transmit information to other channel members and then their local mods react and show for example a notification.

In minetest, you can have a such case, imagine you have 2-3 miners with oredetect mod (which is, by flavour limited to 15 nodes around), your mod send a channel notification hey i found diamond ore at x,y,z coordinates !) and then your friends can come to your location and mine with you . It's a nice usecase, and the startup of real multiplayer gaming without costing anything on server

@mark-otaris

This comment has been minimized.

Copy link

commented Jul 19, 2017

If channels for client-to-client messages are implemented, that's true, but mod channels would still not be useful for any of the use cases in my comment.

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

I disagree with CSM being used for client provided mods. Subgames should be in control of the gameplay whilst being connected to a server. None of those usecases seem valid - waypoints should be a server side mod, and so should area commands. Setting home automatically is a hack and also potentially cheating. Setting colors could be done with a server side mod or a client side feature.

but mod channels would still not be useful for any of the use cases in my comment.

False, a default channel could be used for CSM to request the execution of commands

@rubenwardy rubenwardy closed this Jul 19, 2017

@rubenwardy rubenwardy deleted the rubenwardy:revert_csm_chat branch Jul 19, 2017

@paramat paramat removed this from In Progress in Server-sent Client Side Modding Jun 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.