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

Allow scripts to get the client protocol version in non-debug builds. #5649

Merged
merged 1 commit into from Apr 27, 2017

Conversation

red-001
Copy link
Contributor

@red-001 red-001 commented Apr 23, 2017

example usecase: it could be used by a mod that manipulates the chat to check if the client will display the chat message to the client before sending it to the server.

@sfan5
Copy link
Member

sfan5 commented Apr 23, 2017

This needs careful consideration
usecase?

@sfan5 sfan5 added @ Script API Feature ✨ PRs that add or enhance a feature labels Apr 23, 2017
@shacknetisp
Copy link
Contributor

This is something I would appreciate, currently I had to enable this by editing the source and recompiling to continue supporting both older and new clients with the installed chat mods. Otherwise there's no way to tell if the client will echo their chat automatically or not.

Perhaps a more detailed table would be better than just the raw protocol, however, basic example that I could use:

{
 self_chat_echo = true, -- Will client echo their own chat?
 formspec_close = true, -- Will client close formspecs on ""?
}

@nerzhul
Copy link
Member

nerzhul commented Apr 24, 2017

in CSM can be useful, but we have MT version too, you don't need to know protocol version, release version should be the norm

@SmallJoker
Copy link
Member

@nerzhul I think it's better to use the protocol version here. It clearly shows what the client supports if there were changes. That would make it work better for dev clients, where it's not known what changes it uses.

@nerzhul
Copy link
Member

nerzhul commented Apr 25, 2017

okay

@rubenwardy
Copy link
Member

rubenwardy commented Apr 25, 2017

I'd prefer a feature query system for clients. This would be implemented using protocol version under the hood, but mods wouldn't need to be exposed to that.

Checking against a protocol version is not a very nice API, it's much better and way more readable to check a feature

if player:has_feature("formspec_close_on_empty_string") then
    minetest.show_formspec(player:get_player_name(), "", "")
else
    minetest.show_formspec(player:get_player_name(), "", "size[1,1]button_exit[1,1;1,1;close;close]")
end

It also means that if we ever get the client to declare the features it supports, then mods won't be broken.

@SmallJoker
Copy link
Member

@rubenwardy This way is somewhat KISS from C++ side but I agree, checking against a table of features like in minetest.features is easier to understand for modders that aren't familiar with the protocol versions.
An issue that might appear with that is, that it gets forgotten in new added features with protocol bumps, resulting in outdated information.

@rubenwardy
Copy link
Member

An issue that might appear with that is, that it gets forgotten in new added features with protocol bumps, resulting in outdated information.

minetest.features has the same problem - it's better to make adding feature flag a merging requirement somehow (although you can't CI this).

@nerzhul nerzhul added this to the 0.4.16 milestone Apr 27, 2017
@nerzhul nerzhul merged commit 1ef9eee into minetest:master Apr 27, 2017
@rubenwardy
Copy link
Member

rubenwardy commented Apr 27, 2017

Erm, you didn't address my comments. Checking the protocol version is a bad API.

@nerzhul
Copy link
Member

nerzhul commented Apr 27, 2017

@rubenwardy i think both are useful if needed

@SmallJoker
Copy link
Member

The suggested function has_feature looks very promising but until someone's willing to implement that (what sometimes might take a very long time), it seems fine to offer at least one way to check the features.
At least this pull wasn't stalling ;)

@bigfoot547
Copy link
Contributor

@nerzhul why "Aborted"?
Should be in "Done"

@nerzhul nerzhul moved this from Aborted to Done in Server-sent Client Side Modding Jul 24, 2017
@nerzhul
Copy link
Member

nerzhul commented Jul 24, 2017

i think triage error, but you arevery late: p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants