Stargate Rewritten

Stargate Rewritten

241 Downloads

Assorted Nitpicks [TMS]

Pheotis opened this issue ยท 20 comments

commented

Assorted minor nitpicks found via the current TMS sweep.
This will be edited over time.

  • [222175] Under Aesthetic Tweaks in the default config, the comment mentions that GLOW_INK_SACK is a valid colour modifier. That feature was removed.
    • image
  • [222176] Under the default config, running on the craft bukkit 1.18 TMS mock, the default text colour for birch signs is white.
    • image
  • [222175] Both the supported languages list and the link should be updated.
    • I recall some new localisations having been added.
    • We are now using crowdin.stargate.thorinwasher.dev (although we should consider buying a domain eventually)
    • image
  • [222179] Importing from Knarvik's fork prints a user-level warning... is this normal/intended?
    • image
  • [222191] When a user creates a blank portal without central permissions, it should just default to the lowest network they have perms for (in this case, personal)
    • This error appears for the following portal:
Name1
Name2
  
flags
  • This error should only print if they try (central) or central
  • image
commented

Going through an end_portal results in an advancement.
Can this be suppressed?

image
[TMS 222179]

commented

Wait, so we don't support glowing signs at all anymore?

commented

They kinda look odd, as we use Bungee chatcolors as coloring, while the glowing highlight depend on dyecolor. They might not match. But is this still a wanted feature?

commented

They kinda look odd, as we use Bungee chatcolors as coloring, while the glowing highlight depend on dyecolor. They might not match. But is this still a wanted feature?

It is a feature in my fork, together with being able to specify the colors used for every type of sign individually. I haven't experienced any weird-looking colors myself. In fact, some of the colors are quite nice:
2022-06-07_15 49 14
2022-06-07_15 49 17
2022-06-07_15 49 24

commented

Okay, so that's a thing then

commented
  • [222179] Importing from Knarvik's fork prints a user-level warning... is this normal/intended?

    • image

What logging level do you have in the config, is it Levlel.FINE?

commented
  • [222179] Importing from Knarvik's fork prints a user-level warning... is this normal/intended?

    • image

What logging level do you have in the config, is it Levlel.FINE?

Was mostly just wondering if that error was an issue or not.
If it could be an issue, the current log level is fine.
if it isn't an issue, FINE is probably a better level.

commented

It's not an issue, I meant that this "error" will get thrown if you have debugging enabled

commented

If the error says Index already exists, there is no problem. If it says anything else, there is a problem. It's a consequence of the lazy handling of duplicate indexes.
It is potentially an issue, but not necessarily.

It already has/is Level.FINE.

This is not a specific issue to my fork. It's printed every time createTables() is run after the first run.

commented

If the error says Index already exists, there is no problem. If it says anything else, there is a problem. It's a consequence of the lazy handling of duplicate indexes. It is potentially an issue, but not necessarily.

It already has/is Level.FINE.

This is not a specific issue to my fork. It's printed every time createTables() is run after the first run.

Ah, I forgot that the instance was already running at Level.FINE
That makes the 4th item a total non-issue; going to mark that part as resolved.

commented

When applying glow ink to the sign, should the portal revert to black as its color // remove default coloring?

commented

When applying glow ink to the sign, should the portal revert to black as its color // remove default coloring?

If it's dyed, it should just have the highlighted version of the dye. If it's not though, you could either force the color to black or display it as-is, though it becomes kind of unreadable if highlighted and not dyed. I guess the hightlighting itself is trying to highlight black no matter which color we display.

Users must be able to use INC_SAC as well, to remove the dye.

commented

I guess the hightlighting itself is trying to highlight black no matter which color we display.

Exactly. It can for example look like this:
image

So here the sign is colored black, which makes it go to the default coloring which in this case is light blue. It would be better if the line drawer defaulted back to this:
image

commented

I guess the hightlighting itself is trying to highlight black no matter which color we display.

Exactly. It can for example look like this:

So here the sign is colored black, which makes it go to the default coloring which in this case is light blue. It would be better if the line drawer defaulted back to this:

For my fork, I used the inverse of the dye color for the highlighting, which generally gives some quite nice colors, especially for glowing signs. Could we add that behavior, or at least make it an option?

/**
     * Inverts the given color
     *
     * @param color <p>The color to invert</p>
     * @return <p>The inverted color</p>
     */
    public static Color invert(Color color) {
        return color.setRed(255 - color.getRed()).setGreen(255 - color.getGreen()).setBlue(255 - color.getBlue());
    }

    /**
     * Gets the chat color corresponding to the given color
     *
     * @param color <p>The color to convert into a chat color</p>
     * @return <p>The resulting chat color</p>
     */
    public static ChatColor fromColor(Color color) {
        return ChatColor.of(String.format("#%02X%02X%02X", color.getRed(), color.getGreen(), color.getBlue()));
    }
commented

That would be interesting! Let's see the current options in this version.

signStyle:
  defaultForeground: BLACK
  defaultBackground: WHITE

  # [1: foreground, 2: gate type, 3: grey, 4: background]
  pointer: 2

  # [1: selected gates coloured, 2: all gates coloured, 3: no gates coloured]
  listing: 2

So if we have pointer option number 4, then do the inversion thing for the pointers, if the sign is not black with default coloring

commented

That could work. I really love some of the contrasts it creates, so I don't want to lose it for my server.

commented

For the language point on this thread. It might be a good idea to push that until alpha release

commented

Also for [222175], should the displayed languages always use the full language codes (nb-NO, en-US), or should the shortcuts (en, nn) be used whenever available?

commented

Also for [222175], should the displayed languages always use the full language codes (nb-NO, en-US), or should the shortcuts (en, nn) be used whenever available?

Shortcuts should be used whenever available.

Anyways, as that nitpick is a formatting issue, probably best to delay dealing with that until the alpha release in case any new language translations come in.

As such, I think it is safe to close this issue now.

commented

It seems PlayerAdvancementDoneEvent exists in the API, but it cannot be cancelled, and it doesn't seem like it can be modified.

Paper has an event that might be used: PaperMC/Paper#978