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

RFC 0057: Configuration metadata API #57

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

kvverti
Copy link

@kvverti kvverti commented Jun 4, 2022

Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Unfortunately, I'm not well-versed enough in the technical side of things to critique possible implementations.

Copy link
Contributor

@Haven-King Haven-King left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec certainly covers a lot of the bases, but I feel that it overcomplicates some things while leaving others out.

FREE_TEXT,
SLIDER,
NUMERIC,
UNORDERED_COLLECTION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All config values supported by loaders config API are inherently ordered. Since the behavior of an unordered collection is undefined and I struggle to think of when any of the possible behaviors might be useful, I don't think it really needs to be included.

#### Widget Type

```java
public enum WidgetType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think supporting custom widget types is important. If somebody wants to implement a color wheel widget for their config option, they should be able to do so. Even if their widget doesn't appear alongside other widgets, a solution that opens another screen to edit the option would seem reasonable to me. I think restricting widgets to a small set of types will lead to some less-than-optimal end user experiences.


### Definitions
- **Setting**: A single value and its associated metadata.
- **Category**: A logical grouping of Settings. A Setting may be a member of multiple Categories.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the term "category" here. I would ordinarily assume that each setting belongs to only one category, similar to how game rules have a single category or biomes have a single category. I'm not sure what a better word would be, but wanted to open the discussion.

ORDERED_COLLECTION;
}

MetadataType<WidgetType, ?> WIDGET_TYPE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important that no matter what our system ends up being, we provide reasonable default behaviors for any type. This seems like it would be a good place to note that.

Comment on lines +74 to +78
If a Setting's Widget Type is `UNORDERED_COLLECTION` or `ORDERED_COLLECTION`, additional data is stored `WIDGET_ELEMENT_TYPE`.

```java
MetadataType<WidgetType, ?> WIDGET_ELEMENT_TYPE;
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary at all. We should have one standard way of displaying collections, and collections should simply define the widget type used by all of their children.

MetadataType<WidgetType, ?> WIDGET_ELEMENT_TYPE;
```

#### Value Converter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't think this is necessary. WIDGET_TYPE should just be a MetadataType<Function<TrackedValue<?>, Widget>>, with different kinds of widgets having either constructors or helper methods for different parameter types. ToggleWidget for instance could have the following method:

public static Widget create(TrackedValue<?> value) {
  Object defaultValue = value.value();

  if (defaultValue instanceof Boolean) {
    return new ToggleWidget(value.value());
  } else if (defaultValue instanceof Enum e && e.values().length == 2) {
    return new EnumToggleWidget(value.value());
  } else if (...) {
  
  } else {
    throw new WidgetCreationException(...); // Might be better to have some sort of result class instead of throwing errors, but ¯\_(ツ)_/¯ 
  }
}

The value would then simply call

builder.metadata(WIDGET_TYPE, builder -> builder.factory(ToggleWidget::create))

#### Environment Policy

```java
public enum EnvironmentPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLIENT_ONLY and SERVER_ONLY are functionally identical. I would therefore rename this class to SyncPolicy or something along those lines and condense those two options to NONE, with SERVER_SYNCED becoming S2C and CLIENT_SYNCED becoming C2S. Additionally, I might add a P2P sync type that shares user configs with other clients in a way that allows them to be accessed on a per-player basis. This has a lot of valuable implications for user-set display information (nicknames come to mind).

@Akarys42 Akarys42 changed the title Configuration metadata API RFC 0057: Configuration metadata API Apr 14, 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.

3 participants