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

Updating Skyblock Joining & Leaving Logic, Fixed Location updates, Added Logging. #12

Merged
merged 31 commits into from
May 29, 2023

Conversation

wiviw5
Copy link
Contributor

@wiviw5 wiviw5 commented May 21, 2023

This updated code is for a new bit of logic where Detection is based on what the active scoreboard name is, rather than the old system of updating based on reading a line from the scoreboard (which broke during SKIBLOCK).

How it works:
Joining to a Hypixel lobby gives us for example Prototype as a new name. Telling us that the client no longer is within Skyblock.

Joining into Skyblock, we receive two Scoreboards, one named health and the other SBScoreboard, this also goes along with transferring islands, we also receive this set of packets.

We ignore health as this can lead to false positives for other gamemodes (As it is a common name for scoreboard names)

Next, we check for if it contains SBScoreboard and if it does, then we interact with the event, and we are officially "on Skyblock"

lastly, we check for any other Scoreboard, if we do not find SBScoreboard but another one, we interact with the Leave Skyblock event, and are "off Skyblock"

Currently known edge cases
One edge case of this code is that if we go straight to limbo (skipping any hub/lobby) The "leave" code is never activated, and we continue to think we are still on Skyblock, even if we leave and go to another server without a scoreboard or to a singleplayer world without a scoreboard.

So, this would affect a very minor subset of servers/singleplayer worlds, however, can be fixed with a simple IP check for *.hypixel.net (Not implemented) which would then only affect people who get booted straight to limbo. Please let me know if anyone has any solutions to this.

Location Changes
Updated the location change code to the function of onScoreboardDisplay, as it needs to update on every single Scoreboard update currently.

Logging Changes
Added logging for new Location change code, along with the join/leave Skyblock code.

In the future, this should be behind a toggle for developers for simply debugging if anything goes wrong with this code.

wiviw5 added 2 commits May 21, 2023 00:01
Previously discussed with another developer a better way to determine when a client was joining a new skyblock world. This triggers on every single new join of world. Making sure that mods get the most up-to-date info from the client.

Added some simple logging statements as well to further troubleshoot & test the aspects of Join & leave functions.

Lastly, I moved the Location updateing code up to the Objective Update class, as it functioning requires updating on every objective update received from the client.

SkyblockCore.java
Added some simple logging to Location change event registry for testing to make sure it works correctly.
Previously discussed with another developer a better way to determine when a client was joining a new skyblock world. This triggers on every single new join of world. Making sure that mods get the most up-to-date info from the client.

Added some simple logging statements as well to further troubleshoot & test the aspects of Join & leave functions.

Lastly, I moved the Location updateing code up to the Objective Update class, as it functioning requires updating on every objective update received from the client.

SkyblockCore.java
Added some simple logging to Location change event registry for testing to make sure it works correctly.
@KyllianGamer
Copy link
Contributor

Looks fine for the join and leave detection, given it works ofcourse. But for the logging, wouldn't devs just register a listener themselves to get a debug message like that? And furthermore, putting the actual message between "{}" doesn't make a whole lot of sense as those are generally used to log objects. Also when using the LOGGER object, the message is already prefixed with "(skyblockcore)" (the modid), so adding the name of the mod again also does not make that much sense.

@wiviw5
Copy link
Contributor Author

wiviw5 commented May 21, 2023

Looks fine for the join and leave detection, given it works ofcourse.

Haha, forgot to mention, I did some basic testing with Dungeons, Skyblock hub, Personal island, Main Lobby, and Prototype lobby and it worked flawlessly for the logic on the ones I tested here. However I need a bit more testing to make sure Kuudra, Rift, Straight to limbo, and all other Skyblock islands work, but I did not have time for those today.

But for the logging, wouldn't devs just register a listener themselves to get a debug message like that?

Yes, but also, if its just built in, that allows people reporting issues/developers trying to find out why their mods not working with the core mod to do it without recompiling and re-logging into Hypixel.

And furthermore, putting the actual message between "{}" doesn't make a whole lot of sense as those are generally used to log objects.

Only did this for visibility sake temporarily while writing this. In the future, I'd like to have something more professional yes.

Also when using the LOGGER object, the message is already prefixed with "(skyblockcore)" (the modid), so adding the name of the mod again also does not make that much sense.

It in fact did not prefix my logs with that. Examples below:
[23:44:17] [Render thread/INFO]: [SkyblockCore] {SKYBLOCK SB "Joined SB"} SBScoreboard
[23:44:17] [Render thread/INFO]: [SkyblockCore] {Health Scoreboard "Ignored"} health
[23:44:17] [Render thread/INFO]: [SkyblockCore] {Location Changed from: null to: Your Island}
[23:45:26] [Render thread/INFO]: [SkyblockCore] {Other Scoreboard, "Quit SB"} Prototype

As to why it did not. I have no clue. Wasn't due to anything special that I am aware of either in the development environment/client testing I did there. (And yes, I was hoping it would actually prefix it, but since it didn't, I made this change to be able to spot it within my logs for the time being.)

@axlecoffee
Copy link
Contributor

didnt mean to even push here but

chAnGeS

Changes

  • Not working command handling for /sbcore dev handler thing yeah
  • Formatting for the logs (it looks nice I was planning to make dev toggle then push but HERE WE ARE!)

This reverts commit 13b933e.
### Changes

- Changed all of the formatting of joining/leaving skyblock and zone changes to be a bit more simplified
- Added a dev mode (currently there is no way to toggle this, to turn this off go to `src\main\...\SkyblockCore.java\` it is currently set to `TRUE`
- @wiviw5 and I talked about adding some sort of ingame command and config, I have been working on this and the pr should come soon enough

I have verifed that I am commiting to pr/12
I have checked and tested my code and it has launched and dev mode has worked both on `TRUE` and `FALSE`
@axlecoffee
Copy link
Contributor

Added better formatting for logs

Changes

  • Changed all of the formatting of joining/leaving skyblock and zone changes to be a bit more simplified
  • Added a dev mode (currently there is no way to toggle this, to turn this off go to src\main\...\SkyblockCore.java\ it is currently set to TRUE
  • @wiviw5 and I talked about adding some sort of ingame command and config, I have been working on this and the pr should come soon enough

I have verifed that I am commiting to pr/12
I have checked and tested my code and it has launched and dev mode has worked both on TRUE and FALSE

SkyblockCore.java
Temporary of course until we move these to a better class. But now the developer mode applies to all logging attempts and now looks like the other developer logs.
@wiviw5
Copy link
Contributor Author

wiviw5 commented May 22, 2023

I fixed the last bit of formatting here, its of course still temporary. But now looks better for transitions between locations.

@axlecoffee
Copy link
Contributor

I fixed the last bit of formatting here, its of course still temporary. But now looks better for transitions between locations.

Thanks! I actually fixed it but when i was re-pr'ing it somehow got deleted

@axlecoffee
Copy link
Contributor

If you want you can start working on the command (or anyone else) since I can probably finish the config

@Harry282
Copy link
Contributor

that's crazy

@axlecoffee
Copy link
Contributor

that's crazy

👍 my phone vibrated for you, good job!

@Harry282
Copy link
Contributor

👍 my phone vibrated for you, good job!

got an email for that, good job!

## Additions
- Config creator and handler in JSON
- Added a dev toggle and location toggle
## Changes
- Merged formatting used in the last commit in this pr from @wiviw
- Changed a few logging things to include MODID

I have tested my code and it has launched with all toggles tested
I have changed where I am commiting
I have added information on how to add a config option in ModConfig.java

### TODO for this pr
- Add a command ingame to toggle
- at some point documentation...
@axlecoffee
Copy link
Contributor

Added a config file, and formatting

Additions

  • Config creator and handler in JSON
  • Added a dev toggle and location toggle

Changes

  • Merged formatting used in the last commit in this pr from @wiviw
  • Changed a few logging things to include MODID

I have tested my code and it has launched with all toggles tested
I have changed where I am commiting
I have added information on how to add a config option in ModConfig.java

TODO for this pr

  • Add a command ingame to toggle
  • at some point documentation...

axlecoffee and others added 2 commits May 22, 2023 19:18
…king still.

No, it's not coded nicely. However, it works.

Also, moved the config loading & Command registry to the correct areas of the main initialization, otherwise we get the wrong information.

I also fixed the developer logger information from getting out of date data from an initial load object, to one that grabs the most up-to-date info from the loaded config. (it does not load from disk every time, just load from memory)

SkyblockCoreCommand.java

Completely new class. This is in basic the most simple reload config command I could make.

In the future, we will have different `/skyblockcore <subcommand> <options>`

Under this, it should have:

- reload (Reloads from the config file)
- nbt (Similar to the "/sba dev" or "/skytils dev nbt" commands, it grabs data from hovered items/entities. Useful for devs and curious people.)
- dev (Shows in depth data about all developer related aspects, such as the dev packets.)
- location (Shows in depth location data being handled and shown to mods)
@wiviw5
Copy link
Contributor Author

wiviw5 commented May 23, 2023

Fixed some config file shenanigans & tested out a new command. (made sure that when checking if developer/location feedback mode was enabled was actually respected.)

(Note, tested on development environment & Real Client.)

Additions

Added a local only command to do testing with, and to eventually give users feedback.

Currently looking at:
/skyblockcore <subcommand>

With the subcommands being:

  • reload (For hot reloading of the file if manually edited.)
  • nbt (Currently not implemented, for future nbt grabbing from items/entities.)
  • dev (Not implemented, for future enable & disable of developer feedback logs.)
  • location (Not implemented, for future enable & disable of developer feedback logs.)

Changes

  • Changed a few bits of config checking to make sure changes to the config were respected when reloaded.
  • Made sure config loading & Command registration was done at the proper times in loading.

TODO

  • Better feedback to the player on the commands. (Optimally, I would like every single thing we send to the players chat window in translatable text.)
  • Implementation of the commands

axlecoffee and others added 4 commits May 23, 2023 10:42
Co-authored-by: Ascynx <78341107+Ascynx@users.noreply.github.com>
…nd.java

Co-authored-by: Ascynx <78341107+Ascynx@users.noreply.github.com>
…nd.java

Co-authored-by: Ascynx <78341107+Ascynx@users.noreply.github.com>
…nd.java

Co-authored-by: Ascynx <78341107+Ascynx@users.noreply.github.com>
TGWaffles and others added 2 commits May 23, 2023 17:23
@TGWaffles
Copy link
Member

Fixed the (sub-island) location listener.

It used to listen only for objective updates, but the location isn't actually an objective name - instead it's a team name.

Now we only update the location when we actually receive a team update packet from Hypixel with the location character (U+23E3) as the first character (excluding whitespace).

Works very reliably, and updates instantly when the player location changes in scoreboard. Added bonus effect of partial location updates as Hypixel partially updates the location (but these only persist for a few milliseconds until Hypixel sends the full update).

@Ascynx Ascynx dismissed their stale review May 23, 2023 17:02

Fixed

### Changes
- Made the TITLE string public
- Made scoreboard strings public
- Removed the private versions of ^
- Removed some random lines blankness
- Formatted a tiny bit of logging and changed a few things
### Additions
- Commands now have ingame replies (No color yet, and just basic they still dont do stuff)

### TODO
- Make commands do stuff
- Add color to commands
- change around some logging
- Fix config being nullable
- add a time calc for reloading config
- add a way to reload config
- fix all of the headaches with LOGGER sometimes showing weird stuff
- Fix issues with location

I have checked that my code works in developer launch and normal minecraft
@axlecoffee
Copy link
Contributor

Changes

  • Made the TITLE string public
  • Made scoreboard strings public
  • Removed the private versions of ^
  • Removed some random lines blankness
  • Formatted a tiny bit of logging and changed a few things

Additions

  • Commands now have ingame replies (No color yet, and just basic they still dont do stuff)

TODO

  • Make commands do stuff
  • Add color to commands
  • change around some logging
  • Fix config being nullable
  • add a time calc for reloading config
  • add a way to reload config
  • fix all of the headaches with LOGGER sometimes showing weird stuff
  • Fix issues with location

I have checked that my code works in developer launch and normal minecraft
image

@wiviw5
Copy link
Contributor Author

wiviw5 commented May 23, 2023

Fixed up player checks.

TGWaffles and others added 3 commits May 23, 2023 18:38
…skyblock.

This removes a single string comparison now that we check for anything but specific strings associated with skyblock.
@wiviw5
Copy link
Contributor Author

wiviw5 commented May 23, 2023

Changed around format of getting whether the user has joined or left Skyblock.

This removes a single string comparison now that we check for anything but specific strings associated with Skyblock.

@axlecoffee
Copy link
Contributor

Additions

  • Made a new class for config stuff to remove it from the main class
  • io.github.skyblockcore.event.ConfigManager
  • Made all the commands (aside from NBT) do stuff

Changes

  • Changed all old config uses to use the new method
    image

Known issues

  • I somehow caused see image

@axlecoffee
Copy link
Contributor

Additions

  • Log filtering
  • log filter toggle command
  • log filter config lines
  • test command!
  • cute catboys

Changes

  • Removed the femboys from an old update as it was ruining the motivation of the team

I have tested this and it has launched and built in dev and in minecraft

TODO

  • Fix config not allowing new code to be printed without hard reset
  • colors 🌈 🏳️‍🌈 🏳️‍⚧️ 👑 💅 💅 💅 💅 👄 💋 💄

@wiviw5 wiviw5 mentioned this pull request May 24, 2023
4 tasks
@TGWaffles
Copy link
Member

Is this ready for reviews?

@axlecoffee
Copy link
Contributor

Is this ready for reviews?

You have pending changes iirc

@CraftedFury CraftedFury dismissed TGWaffles’s stale review May 29, 2023 01:05

Opened an issue.

@CraftedFury CraftedFury merged commit 47fb002 into SkyBlock-Central:main May 29, 2023
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.

7 participants