Conversation
|
There is a very marginal problem that writing changes mid-game via code modding may cause the client to asynchronize with the server and disconnect. This is mainly due to #12772 #10203 ; so here it needs to do the same thing in the server source, make |
Regalis11
left a comment
Regalis11
left a comment
There was a problem hiding this comment.
Spotted one oddity in the changes. I also believe there's an issue with syncing the new values in multiplayer as zhu-rengong commented: since they only exist client-side, trying to sync properties of the label will now cause errors in multiplayer due to the mismatching number of editable properties between the server and the clients.
| }; | ||
| if (!Fields.ContainsKey(property.Name)) { Fields.Add(property.Name.ToIdentifier(), new GUIComponent[] { propertyTickBox }); } | ||
| return propertyTickBox; | ||
| } |
There was a problem hiding this comment.
Unclear why this method was modified (seems unrelated to the rest of the changes in the PR) or what was actually modified about it.
There was a problem hiding this comment.
Seems like the new commit has made it show the changes.
From the looks of it, it seems autoformatting has struck again, fixing broken spacing.
Most likely not a problem, but can be reverted if necessary.
Regalis11
added
Feature request
Regalis11
left a comment
Regalis11
left a comment
There was a problem hiding this comment.
I feel there are still some issues with the way the new properties have been implemented. I also don't fully understand the GUIBridge class. I would recommend syncing the font in some other way: it doesn't seem ideal to me to add this sort of a "mock" class that doesn't actually fully work outside this specific context.
| { | ||
| public readonly Identifier Identifier; | ||
| } | ||
| } |
There was a problem hiding this comment.
The purpose of this class is not at all clear without diving into the code and seeing how it's used (and even then it's not that clear unless you happen to know what issue it was intended to address). At the very least I think it needs some comments explaining what it's for.
I'm also not sure this works correctly. I don't see any place where the server would add values to the Fonts dictionary.
There was a problem hiding this comment.
I changed to defining them as partial classes in the shared source, and included the addition of fonts as well.
| public Alignment TextAlignment { get; set; } | ||
|
|
||
| [Editable, Serialize("UnscaledSmallFont", IsPropertySaveable.Yes, "The label's font.")] | ||
| public GUIFont Font { get; set; } |
There was a problem hiding this comment.
It seems potentially error-prone to me that we'd maintain two sets of these properties. It would be very easy for someone to do the mistake of editing these in the client project without realizing the changes need to be done to the server project as well (because surely UI stuff like this is client-only, so there's no need to touch the server code?). This also introduces some unnecessary code duplication.
I think it'd be preferable to define these in the shared project.
There was a problem hiding this comment.
Done, all duplicated properties are now defined in the shared source.
Regalis11
left a comment
Regalis11
left a comment
There was a problem hiding this comment.
Sorry, I think I should've been clearer in my previous review: by suggesting that we shouldn't implement this sort of a mock font and style classes, I didn't mean that we should implement "full" support for GUIStyles and Fonts server-side or move the classes to shared code. In my opinion this latest commit vastly increases the scope of the PR: it includes hundreds of lines of new changes by introducing entirely new kind-of-content-types server-side, and it's unclear at least to me to what extent they actually work or are used server-side (I would suspect they don't really, and a lot of it is essentially dead code).
As far as I can see, the only thing we need is a way for the server to communicate the identifier of the selected font to the clients. I don't think we need to go this far to do that, I believe something like a server-side property of the type Identifier that corresponds to the client-side Font property would suffice. Or maybe it could just be a shared property of the type Identifier, and the setter would just find the appropriate font based on the identifier?
3 participants
This PR allows submakers and modders to: