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

Make fields public for ServerListPingEvent #75

Merged
merged 43 commits into from
Dec 18, 2024

Conversation

theaddonn
Copy link
Contributor

This pr just makes some fields public for the ServerListPingEvent. This is helpful since you may want to modify stuff like the port or Minecraft Version shown.
This PR is likely missing some stuff since this would also require to change some underlying python bindings, but eh..

Also, any reason we are using .h instead of .hpp?

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 12.29630% with 592 lines in your changes missing coverage. Please review.

Project coverage is 8.02%. Comparing base (b8e78c6) to head (219c890).
Report is 149 commits behind head on main.

Files with missing lines Patch % Lines
src/endstone_core/ban/player_ban_list.cpp 45.55% 29 Missing and 69 partials ⚠️
src/endstone_core/packs/endstone_pack_source.cpp 0.00% 73 Missing ⚠️
src/endstone_core/plugin/plugin_manager.cpp 0.00% 45 Missing ⚠️
src/endstone_core/command/defaults/ban_command.cpp 0.00% 41 Missing ⚠️
src/endstone_core/player.cpp 0.00% 37 Missing ⚠️
src/endstone_core/level/level.cpp 0.00% 33 Missing ⚠️
src/endstone_core/server.cpp 0.00% 33 Missing ⚠️
src/endstone_core/command/command_map.cpp 0.00% 31 Missing ⚠️
src/endstone_core/lang/language.cpp 0.00% 27 Missing ⚠️
.../endstone_core/command/defaults/pardon_command.cpp 0.00% 26 Missing ⚠️
... and 13 more
Additional details and impacted files
@@           Coverage Diff            @@
##            main     #75      +/-   ##
========================================
+ Coverage   7.29%   8.02%   +0.73%     
========================================
  Files         55      62       +7     
  Lines       3071    4384    +1313     
  Branches    1349    1895     +546     
========================================
+ Hits         224     352     +128     
- Misses      2669    3776    +1107     
- Partials     178     256      +78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@theaddonn
Copy link
Contributor Author

This PR instead of making the needed fields public, adds methods to set the local port (ipv4 & pv6) as well as the guid

@wu-vincent
Copy link
Member

wu-vincent commented Oct 16, 2024

Thank you for the pull request! These fields were intentionally kept read-only, as modifying these values doesn't affect their appearance in the server list. I am wondering about the practical usage of making these fields writable with these setters?

@Dingsel
Copy link

Dingsel commented Oct 16, 2024

Hey, the reason why local port should be writable as there is a chance for a port mismatch to happen when dealing with proxies. As when the proxy is on one port while the server is on another port the server MOTD ping will container the servers port and not the port of the proxy. This can confuse clients into believing the server does not exist in the server list.

@wu-vincent
Copy link
Member

@Dingsel That makes sense to me, although I think the proxy should ideally handle the ping response from backend servers, rather than the backend servers managing it directly. But I assume the proxies aren't that advanced yet.

@theaddonn One more thing before I merge this PR—could you also add the Python bindings for these setters?

@theaddonn
Copy link
Contributor Author

@wu-vincent The proxy are that advanced, but it introduces a lot more overhead.. which can easily be avoided by just changing the needed data on the backend server directly.

wu-vincent and others added 23 commits December 6, 2024 11:15
* fix: fix material.h and material_type.h

* refactor: rename canDropWithAnyTool to requiresCorrectToolForDrops

* refactor: remove material related things since these things are unused

* docs: add a FIXME notice to biome.h
…ile (EndstoneMC#110)

* fix: fix EndstonePluginManager::loadPlugin EndstoneMC#98

* refactor: improve plugin loading logic in EndstonePluginManager

* refactor: revert back to range-based loop for plugin loading

---------

Co-authored-by: Vincent <[email protected]>
…patch-2

# Conflicts:
#	include/endstone/event/server/server_list_ping_event.h
@wu-vincent wu-vincent merged commit 45a5c55 into EndstoneMC:main Dec 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants