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

Destroyable - Add new sparks parameter value of "near" #1405

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

arcadeboss
Copy link
Contributor

Adds a new parameter "sparks-match" to Destroyable

"sparks" must be active for "sparks-match" to have any effect

With the default value of "true" there is no change to current behavior for Destroyable sparks.

With value of "false" PGM will skip the far (+64 blocks away) logic

Purpose is to reduce the "noise" for players not actually near the action, especially for large Destroyables

Tested and verified that sound behavior changed for players and observers

Signed-off-by: arcadeboss <bluejupiternetworks@gmail.com>
@Pablete1234
Copy link
Member

I think it needs a better name, maybe consider allowing sparks to have true, false, and 'near' that disables the global sound instead of a diff attr

@arcadeboss
Copy link
Contributor Author

I think it needs a better name, maybe consider allowing sparks to have true, false, and 'near' that disables the global sound instead of a diff attr

I considered that, but felt wrong to change the type, but if you think it's okay, then alright.

@Pablete1234
Copy link
Member

but felt wrong to change the type

as long as it's backwards-compatible in xml, i don't think it's an issue

Signed-off-by: arcadeboss <bluejupiternetworks@gmail.com>
@arcadeboss
Copy link
Contributor Author

as long as it's backwards-compatible in xml, i don't think it's an issue

I have updated the code to use an Enum to enforce the 3 spark type values of "true", "false" and "near"

The code is backwards compatible, so current uses of "true" and "false" remain the same, and "near" provides the new logic of having sparks, but no far match sounds.

There is a check for valid sparks values. and a corresponding exception thrown if non-valid value provided.

@arcadeboss arcadeboss changed the title Destroyable - Add sparks-match parameter (signed) Destroyable - Add new sparks parameter value of "near" Oct 1, 2024
Signed-off-by: arcadeboss <bluejupiternetworks@gmail.com>
@arcadeboss
Copy link
Contributor Author

Updated from review comments. Abandoned aliasing, because couldn't make it work with parseEnum, but got everything else.

Signed-off-by: arcadeboss <bluejupiternetworks@gmail.com>
@Pablete1234
Copy link
Member

Abandoned aliasing, because couldn't make it work with parseEnum, but got everything else.

Should be working out of the box, it's built-in already as long as you implement the aliasing interface in pgm like settings do

@arcadeboss
Copy link
Contributor Author

arcadeboss commented Oct 5, 2024

Abandoned aliasing, because couldn't make it work with parseEnum, but got everything else.

Should be working out of the box, it's built-in already as long as you implement the aliasing interface in pgm like settings do

This is what I implemented. It complies, but at run time gives following error:

'sparks' attribute of 'destroyables' > 'destroyable' element @ line 2002 to 2004: Invalid format: 'false' is not a type.sparkstype (did you mean 'none'?)

See anything wrong?

public static enum SparksType implements Aliased {
NONE("false"),
NEAR("near"),
ALL("true");

private final String name;

private SparksType(String name) {
this.name = name;
}

public String getName() {
return name;
}

@NotNull
@OverRide
public Iterator iterator() {
return Iterators.forArray(name(), name);
}
}

@arcadeboss
Copy link
Contributor Author

The first attempt at iterator() above was based on the MapPoolType class enum which has the same iterator method that you shared.

But you, also mentioned following a settings enum, so I tried implementing like SettingKey class does it, but it gets the same results at run time that first one did. It looks like this:

public static enum SparksType implements Aliased {
NONE("false"),
NEAR("near"),
ALL("true");

private final List aliases;

SparksType(String name) {
this(Collections.singletonList(name));
}

SparksType(List aliases) {
this.aliases = ImmutableList.copyOf(aliases);
}

@NotNull
@OverRide
public Iterator iterator() {
return aliases.iterator();
}
}

@Pablete1234
Copy link
Member

Looked into why aliased enums were being rejected myself and submitted a fix into the PR myself, should be good to go now

Signed-off-by: Pablo Herrera <pabloherrerapalacio@gmail.com>
@Pablete1234
Copy link
Member

Also note, for backwards-compat we need to support "true", "yes", and "on" variants as those are all valid boolean parsing in pgm xml (ie: you could do sparks="off" or sparks="on" and they'd work the same as true/false)

@arcadeboss
Copy link
Contributor Author

Wow, that's awesome, thank you for those updates!

I was poking around in the text parser, but never would have figured out those changes. Nice on the extended enums.

@Pablete1234 Pablete1234 merged commit ddae132 into PGMDev:dev Oct 9, 2024
2 checks passed
@arcadeboss arcadeboss deleted the sparks-match branch October 10, 2024 02:56
TheRealPear pushed a commit to OvercastCommunity/CommunityMaps that referenced this pull request Oct 19, 2024
Pablete1234 added a commit that referenced this pull request Nov 30, 2024
Signed-off-by: arcadeboss <bluejupiternetworks@gmail.com>
Signed-off-by: Pablo Herrera <pabloherrerapalacio@gmail.com>
Co-authored-by: Pablo Herrera <pabloherrerapalacio@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants