Conversation
|
I tested this as best I could using 3 instances of Celeste hooked up to a locally hosted server. Stress tests may be in order? |
|
Did a lot of refactoring per some advice on discord, there is no longer a server module needed! |
|
oh you know what i totally forgot to test that before pushing. one second |
|
any updates on this? |
|
Hey, really sorry about the lack of activity 😅 Personally been having some trouble with working on CNet stuff or what to prioritize, but I can probably take a more thorough look at this again over the weekend? This is definitely a solid PR that is 90% where it should be, but I still see some areas I wanna go over. |
RedFlames
left a comment
RedFlames
left a comment
There was a problem hiding this comment.
I don't even know what exactly GitHub does when you "Finish" a Review, but I'm doing it :33:
| using System.Reflection; | ||
|
|
||
| namespace Celeste.Mod.CelesteNet.Client.Components; | ||
| #nullable enable |
There was a problem hiding this comment.
I'm not even sure why Client is the only project that doesn't have nullable context enabled. We should look into if we can turn that on for the whole project and fix warnings, rather than just toggling it for this file alone imo.
|
|
||
| CelesteNetPlayerListComponent.OnGetState += ModifyStateOfPicoPlayers; | ||
| CelesteNetMainComponent.OnSendState += ModifyStateInPico; | ||
| var client = Context?.Client; |
There was a problem hiding this comment.
drop the var and use the Client property of CelesteNetGameComponent?
|
|
||
| CelesteNetPlayerListComponent.OnGetState -= ModifyStateOfPicoPlayers; | ||
| CelesteNetMainComponent.OnSendState -= ModifyStateInPico; | ||
| var client = Context?.Client; |
| private void ModifyStateInPico(DataPlayerState state) | ||
| { | ||
| uint? id = state.Player?.ID; | ||
| if (id == null) return; |
There was a problem hiding this comment.
Just check state.Player == null, drop the local var and possibly combine the two conditional returns
|
|
||
| Logger.Log(LogLevel.DBG, "PICO8-CNET", $"Modifying state!"); | ||
|
|
||
| state.SID = $"PICO-8"; |
There was a problem hiding this comment.
As I already said I think it'd make sense to make this "Celeste/PICO-8", and since the constant shows up in multiple places, make it a member constant variable?
| var x = (float?) nodeData.Get("x") ?? float.NaN; | ||
| var y = (float?) nodeData.Get("y") ?? float.NaN; | ||
| var size = (float?) nodeData.Get("size") ?? float.NaN; |
There was a problem hiding this comment.
use .Get<float>()? 🤔
Unsure why VS then tells me it wouldn't even return a float? but maybe that's nullable-context jank we gotta fix...
| if (_classic == null) { | ||
| Logger.Log(LogLevel.WRN, "PICO8-CNET", "Failed to retrieve Classic"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
if _classic was null, InitData wouldn't have returned true and the previous conditional would've returned, right
|
|
||
| if (Client?.Data?.TryGetBoundRef(player, out DataPlayerState state) == true && !string.IsNullOrEmpty(state?.SID)) | ||
| return $"0 {(state.SID.StartsWith("Celeste/") ? "0" : "1") + state.SID + (int) state.Mode} {player.FullName}"; | ||
| return $"0 {(state.SID.StartsWith("Celeste/") || state.SID == "PICO-8" ? "0" : "1") + state.SID + (int) state.Mode} {player.FullName}"; |
There was a problem hiding this comment.
Change would be avoided by using SID "Celeste/PICO-8"
| @@ -1,4 +1,5 @@ | |||
| using Microsoft.Xna.Framework; | |||
| using Celeste.Pico8; | |||
There was a problem hiding this comment.
Changes in this file are superfluous? The SceneAs vs. is not seems okay although idk about the { return; } instead of newline no-braces ;)
| public static void Print(string text, int x, int y) { | ||
| Print(text, x, y, PicoWhite); | ||
| } | ||
|
|
||
| public static void Print(string text, int x, int y, Color color) { | ||
| var initialX = x; | ||
| for (var i = 0; i < text.Length; i += char.IsSurrogatePair(text, i) ? 2 : 1) { | ||
| var codepoint = char.ConvertToUtf32(text, i); | ||
| if (codepoint == 0x0A) { | ||
| // Newline | ||
| x = initialX; | ||
| y += 5; | ||
| continue; | ||
| } | ||
| var sprite = CharacterSprite((uint) codepoint); | ||
|
|
||
| sprite.Draw(new Vector2(x, y), Vector2.Zero, color); | ||
| x += 4; | ||
| } | ||
| } | ||
| public static void PrintOutlinedCenter(string text, int x, int y) { | ||
| PrintOutlinedCenter(text, x, y, PicoWhite, PicoBlack); | ||
| } | ||
|
|
||
| public static void PrintOutlinedCenter(string text, int x, int y, Color inside, Color outside) { | ||
| Print(text, x - (text.Length / 2 * 4) - 1, y - 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) , y - 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) + 1, y - 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) - 1, y , outside); | ||
| Print(text, x - (text.Length / 2 * 4) + 1, y , outside); | ||
| Print(text, x - (text.Length / 2 * 4) - 1, y + 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) , y + 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) + 1, y + 1, outside); | ||
| Print(text, x - (text.Length / 2 * 4) , y , inside); | ||
| } |
There was a problem hiding this comment.
Could give the Color parameters default values instead of the overloaded methods? 🤷
2 participants
Adds ghosts to PICO-8!
https://www.youtube.com/watch?v=OWObGCg-KDo