Added new property `Viewer` for hologram. #99 #100

BigTows · 2024-05-23T07:57:01Z

Added a new property to the hologram instance.
This property specifies the list of players who are allowed to view this hologram. (Similar to a permission right now.)

If that solution is relevant, I can add command for that field.

BigTows · 2024-05-23T07:57:16Z

via #99

OakLoaf · 2024-05-23T08:15:29Z

I think it would be better to implement something into the current system instead of having 2 different sets of uuids, we could probably make the set of shown players into a map of uuids to boolean. The boolean value being a sort of "force view" which ignores permissions.

There is likely a better solution than this too though

BigTows · 2024-05-23T08:18:40Z

@OakLoaf For memory obviously, but not for logic. This structure will complicate the management and editing of the code.

BigTows · 2024-05-23T08:20:49Z

If you wanna one structure for that. Need create some PlayerContext and control them. For next updates that context can be improved without edit current code.

OakLoaf · 2024-05-23T08:21:11Z

@OakLoaf For memory obviously, but not for logic. This structure will complicate the management and editing of the code.

This current solution is not good for memory and I'm almost certain this isn't the best solution from a logic standpoint either.

Another alternative would be to add a requiresPermission attribute that can be disabled.

BigTows · 2024-05-23T08:54:49Z

Another alternative would be to add a requiresPermission attribute that can be disabled.

Yeah it's another solution by different way.

So how can we fix #99?

OakLoaf · 2024-05-23T09:07:01Z

Another alternative would be to add a requiresPermission attribute that can be disabled.

Yeah it's another solution by different way.

So how can we fix #99?

I think adding a permission option might be the feasible option

BigTows · 2024-05-23T09:15:27Z

Another alternative would be to add a requiresPermission attribute that can be disabled.

Yeah it's another solution by different way.
So how can we fix #99?

I think adding a permission option might be the feasible option

May be convert visibleByDefault to ENUM?
like that:

ALL,
NO_ONE,
PERMISSION_REQUIRED

OakLoaf · 2024-05-23T09:18:48Z

I think if we were to do something like that it'd make more sense to accept a predicate.

BigTows · 2024-05-23T09:21:49Z

I think if we were to do something like that it'd make more sense to accept a predicate.

From? I didn't understand you

BigTows · 2024-05-23T09:24:23Z

Or do you talk about API? Yeah it's possible, propably good idea.

This reverts commit 63b95c9.

OakLoaf · 2024-05-23T09:27:25Z

Only issue regarding that would be that there would need to be some way to save that predicate

OakLoaf · 2024-05-23T09:30:16Z

I personally think we should just stick with the requiresPermission option, if there is an issue in the future where another filter type option is needed then we can reconsider implementing a predicate system.

BigTows · 2024-05-23T10:11:46Z

@OakLoaf check this out please.

OakLoaf

With these changes it would probably be a good idea to rename VisibleByDefault to VisibleTo

OakLoaf · 2024-05-23T10:37:11Z

api/src/main/java/de/oliver/fancyholograms/api/data/property/visibility/Visibility.java

+    @Nullable
+    public static Visibility byString(String value) {
+        return Arrays.stream(Visibility.values())
+                .filter(visibility -> visibility.toString().equalsIgnoreCase(value))
+                .findFirst().orElse(null);
+    }


This should be removed in favour of Enum#valueOf(String#toUpperCase)

valueOf is throwable, for safe parsing null needed.

There should be no instance where it returns null anyway, if it is null wouldn't there be issues in other places? If so the error would be more helpful.

What about Optional ? :)

api/src/main/java/de/oliver/fancyholograms/api/data/property/visibility/Visibility.java

...src/main/java/de/oliver/fancyholograms/api/data/property/visibility/VisibilityPredicate.java

build.gradle.kts

OakLoaf · 2024-05-23T17:57:02Z

src/main/java/de/oliver/fancyholograms/commands/HologramCMD.java

@@ -209,7 +209,8 @@ public boolean execute(@NotNull CommandSender sender, @NotNull String label, @No
                    yield FancyNpcsPlugin.get().getNpcManager().getAllNpcs().stream().map(npc -> npc.getData().getName());
                }
                case "block" -> Arrays.stream(Material.values()).filter(Material::isBlock).map(Enum::name);
-                case "seethrough", "visiblebydefault" -> Stream.of("true", "false");
+                case "seethrough" -> Stream.of("true", "false");
+                case "visiblebydefault" -> new VisibleByDefaultCMD().tabcompletion(sender, hologram, args).stream();


Creating a new command object for every tabcomplete is probably not a good idea, I would just copy the contents of the method over.

This is should be refactored at all.

All CMDs impltement Subcommand with execute and tabcompletion methods.

OakLoaf · 2024-05-23T17:57:51Z

src/main/java/de/oliver/fancyholograms/storage/FlatFileHologramStorage.java

+            final var visibleByDefault = Optional.of(config.getString(
+                    "visible_by_default", DisplayHologramData.DEFAULT_IS_VISIBLE.toString())
+            ).map(Visibility::byString).orElse(DisplayHologramData.DEFAULT_IS_VISIBLE);


It would probably be a good idea to add backwards compatibility and support the old boolean option for a while.

You want to check booleans and convert into Predicate?

BigTows · 2024-05-23T18:31:21Z

With these changes it would probably be a good idea to rename VisibleByDefault to VisibleTo

On java layer or config too?

OakLoaf · 2024-05-23T18:43:29Z

With these changes it would probably be a good idea to rename VisibleByDefault to VisibleTo

On java layer or config too?

I'd say in config too, with backwards compatibility kept if the new one isn't defined

BigTows · 2024-05-23T18:46:45Z

With these changes it would probably be a good idea to rename VisibleByDefault to VisibleTo

On java layer or config too?

I'd say in config too, with backwards compatibility kept if the new one isn't defined

I can but you really want store deprecated fields? Are you sure that it won't confuse users?

OakLoaf · 2024-05-23T18:50:42Z

I can but you really want store deprecated fields? Are you sure that it won't confuse users?

It would likely get automatically updated on auto-save anyways but I think that loading from the old config would mean that the pr is more likely to be merged sooner

BigTows · 2024-05-23T19:00:40Z

I can but you really want store deprecated fields? Are you sure that it won't confuse users?

It would likely get automatically updated on auto-save anyways but I think that loading from the old config would mean that the pr is more likely to be merged sooner

Right now old configuration supported. Boolean converting to Enum on save.

OliverSchlueter · 2024-05-23T19:06:58Z

Maybe we should rename the visibleByDefault command with just "visibility"

BigTows · 2024-05-23T19:43:10Z

Maybe we should rename the visibleByDefault command with just "visibility"

Added migration for field visible_by_default.

After launch old boolean value will be converted to one of the following modes:
ALL - true
PERMISSION_REQUIRED - false

On save visible_by_default will be deleted (on v1 and v2 too).
And new field visibility will be generated.

BigTows · 2024-05-24T13:36:48Z

@OliverSchlueter @OakLoaf Is there something else?

OakLoaf · 2024-05-24T13:43:36Z

Not that I can think of, I will review this after work in a few hours

src/main/java/de/oliver/fancyholograms/commands/hologram/VisibilityCMD.java

src/main/java/de/oliver/fancyholograms/storage/FlatFileHologramStorage.java

BigTows · 2024-05-24T17:33:52Z

@OakLoaf Fixed, check another comments please.

OakLoaf · 2024-05-24T17:34:44Z

All looks good to me

OakLoaf · 2024-05-24T17:35:01Z

Has this been tested in-game to make sure everything works and is backwards compatible?

BigTows · 2024-05-24T17:36:36Z

Has this been tested in-game to make sure everything works and is backwards compatible?

I tested that on v2 configuration works fine for me. Do you have an example of old (v1) configurations?

OakLoaf · 2024-05-24T18:26:20Z

This should do, just teleport it to your location

  test-text:
    type: TEXT
    location:
      world: world
      x: 45.06436912097689
      y: 87.61627563479522
      z: -90.4272584957459
      yaw: -109.71094
      pitch: 90.0
    visibility_distance: -1
    visible_by_default: true
    scale_x: 1.0
    scale_y: 1.0
    scale_z: 1.0
    shadow_radius: 0.0
    shadow_strength: 1.0
    text:
    - Edit this line with /hologram edit text
    - test with more words

BigTows · 2024-05-24T18:33:14Z

Configuration before:

holograms:
  test-text:
    type: TEXT
    location:
      world: world
      x: 45.06436912097689
      y: 87.61627563479522
      z: -90.4272584957459
      yaw: -109.71094
      pitch: 90.0
    visibility_distance: -1
    visible_by_default: true
    scale_x: 1.0
    scale_y: 1.0
    scale_z: 1.0
    shadow_radius: 0.0
    shadow_strength: 1.0
    text:
    - Edit this line with /hologram edit text
    - test with more words

Edit command

Move here

Remove perms

Configuration after stop the server

version: 2 # DO NOT CHANGE
holograms:
  test-text:
    type: TEXT
    location:
      world: world
      x: 81.10001790115858
      y: 75.0
      z: 71.98312464428668
      yaw: -165.9294
      pitch: 21.240873
    scale_x: 1.0
    scale_y: 1.0
    scale_z: 1.0
    shadow_radius: 0.0
    shadow_strength: 1.0
    visibility_distance: -1
    visibility: ALL
    text:
    - Edit this line with /hologram edit text
    - test with more words
    text_shadow: false
    see_through: false
    text_alignment: center
    update_text_interval: -1

OakLoaf · 2024-05-24T18:35:50Z

Very nicee

BigTows · 2024-05-24T18:38:07Z

And one more test :)

Remove perms

Configuration after stop the server

version: 2 # DO NOT CHANGE
holograms:
  test-text:
    type: TEXT
    location:
      world: world
      x: 81.10002136230469
      y: 75.0
      z: 71.98312377929688
      yaw: -165.9294
      pitch: 21.240873
    scale_x: 1.0
    scale_y: 1.0
    scale_z: 1.0
    shadow_radius: 0.0
    shadow_strength: 1.0
    visibility_distance: -1
    visibility: PERMISSION_REQUIRED
    text:
    - Edit this line with /hologram edit text
    - test with more words
    text_shadow: false
    see_through: false
    text_alignment: center
    update_text_interval: -1

BigTows · 2024-05-24T18:43:17Z

@OakLoaf
We forgot about old API compatibility, maybe add Deprecated method

setVisibleByDefault(boolean){
   setVisiible(boolean?ALL:PERMS_REQUIRED):
}

What do you think?

OakLoaf · 2024-05-24T18:56:21Z

Yeahh that's good just pop some spaces in there so it's more readable: value ? ALL : PERMS_REQUIRED

BigTows · 2024-05-24T19:09:15Z

Yeahh that's good just pop some spaces in there so it's more readable: value ? ALL : PERMS_REQUIRED

Yeah :) ofc it's was just a draft code.

I did it.

BigTows · 2024-05-25T01:52:43Z

@OakLoaf While I doing this, I am completely forgot about my case....
MANUAL not working for #99 case :(

Can I add another list of users?

OakLoaf · 2024-05-25T08:00:09Z

I really don't think this added commit should be part of this PR. As far as I am aware Issue #99 was resolved as you can now input a custom predicate.

The only issue you'd face is having it save, the only real fix for that would be to make the visibility enum into some sort of registry where people could register new visibility types, it'd just need to be done before FancyHolograms first loads the holograms.

I'm not sure what @OliverSchlueter thinks of this as it does make it more complex but the manual visibility is just not a good solution in my eyes.

BigTows · 2024-05-25T17:12:26Z

I really don't think this added commit should be part of this PR. As far as I am aware Issue #99 was resolved as you can now input a custom predicate.

It's talk about user frendly API. Do you make them implement the same type of logic every time, in every project, or do you implement once it for them?

OakLoaf · 2024-05-25T17:17:26Z

The whole reason we implemented a predicate was so that we wouldn't have to have 2 viewer lists. 2 lists is not necessary.

BigTows · 2024-05-25T17:21:57Z

The whole reason we implemented a predicate was so that we wouldn't have to have 2 viewer lists. 2 lists is not necessary.

But that second list will be allocated in a lot of projects whos using that plugin. This is balance of the user friendly.

BigTows · 2024-05-25T17:23:46Z

The whole reason we implemented a predicate was so that we wouldn't have to have 2 viewer lists. 2 lists is not necessary.

But that second list will be allocated in a lot of projects whos using that plugin. This is balance of the user friendly.

And you will repeat repeat that logic every time ;/

OakLoaf · 2024-05-25T17:28:31Z

I completely disagree, the entirety of the most recent commit can be replaced with the predicate (player, hologram) -> hologram.isShown(player.getUniqueId())

BigTows · 2024-05-25T17:30:44Z

I completely disagree, the entirety of the most recent commit can be replaced with the predicate (player, hologram) -> hologram.isShown(player.getUniqueId())

Nope, show is service field. Show changing every time then plugin want, you can't control that.
For example, swap world, too far and another stuff, plugin remove player ID from that list.

OakLoaf · 2024-05-25T17:35:20Z

In that case, an exception of some form could be implemented to ensure that they aren't removed in those instances. Although I imagine adding the visibility distance check into the predicate would probably the better solution here.

BigTows · 2024-05-25T17:45:12Z

In that case, an exception of some form could be implemented to ensure that they aren't removed in those instances.

Already, we can listen HologramHideEvent but this is not a solution.

Although I imagine adding the visibility distance check into the predicate would probably the better solution here.

This has a bad effect on performance and the difference in configuration

BigTows · 2024-05-25T17:51:04Z

My point like a user.

I want to control who can and who can't see the hologram, I don't want to control the plugin processes (hiding due to distance or exiting the server)

OakLoaf · 2024-05-25T18:10:20Z

I agree that the issue needs a solution and will happily resolve that myself in a different pr.

I've had a chat with Oliver and if you'd like to revert the most recent commit he'll happily merge this.

This reverts commit 9d2a711.

BigTows · 2024-05-25T19:10:37Z

Okay.

OliverSchlueter · 2024-05-25T19:14:42Z

Thank you for your contribution ❤️!

Added new property Viewer for hologram. FancyMcPlugins#99

63b95c9

Revert "Added new property Viewer for hologram. FancyMcPlugins#99"

388ab51

This reverts commit 63b95c9.

Added predicate for visibility

fad7b0a

OakLoaf suggested changes May 23, 2024

View reviewed changes

Pr fixes.

e6b9ec5

Added migration for visibility to visible_by_default

5561a46

OakLoaf suggested changes May 24, 2024

View reviewed changes

OakLoaf approved these changes May 24, 2024

View reviewed changes

Added old API and improve javadocs.

46a3715

Added list for Manual visibility.

9d2a711

Revert "Added list for Manual visibility."

bce6c0d

This reverts commit 9d2a711.

OliverSchlueter merged commit 337a4ad into FancyMcPlugins:main May 25, 2024

Added new property Viewer for hologram. #99 #100

Added new property Viewer for hologram. #99 #100

Conversation

BigTows commented May 23, 2024

BigTows commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

BigTows commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024 • edited Loading

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

BigTows commented May 23, 2024 • edited Loading

OakLoaf commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

OakLoaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigTows commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

OakLoaf commented May 23, 2024

BigTows commented May 23, 2024

OliverSchlueter commented May 23, 2024

BigTows commented May 23, 2024 • edited Loading

BigTows commented May 24, 2024

OakLoaf commented May 24, 2024

BigTows commented May 24, 2024

OakLoaf commented May 24, 2024

OakLoaf commented May 24, 2024 • edited Loading

BigTows commented May 24, 2024 • edited Loading

OakLoaf commented May 24, 2024

BigTows commented May 24, 2024

OakLoaf commented May 24, 2024

BigTows commented May 24, 2024

BigTows commented May 24, 2024 • edited Loading

OakLoaf commented May 24, 2024 • edited Loading

BigTows commented May 24, 2024 • edited Loading

BigTows commented May 25, 2024 • edited Loading

OakLoaf commented May 25, 2024

BigTows commented May 25, 2024 • edited Loading

OakLoaf commented May 25, 2024 • edited Loading

BigTows commented May 25, 2024

BigTows commented May 25, 2024

OakLoaf commented May 25, 2024 • edited Loading

BigTows commented May 25, 2024

OakLoaf commented May 25, 2024

BigTows commented May 25, 2024 • edited Loading

BigTows commented May 25, 2024

OakLoaf commented May 25, 2024 • edited Loading

BigTows commented May 25, 2024

OliverSchlueter commented May 25, 2024

Added new property `Viewer` for hologram. #99 #100

Added new property `Viewer` for hologram. #99 #100

BigTows commented May 23, 2024 •

edited

Loading

BigTows commented May 23, 2024 •

edited

Loading

BigTows commented May 23, 2024 •

edited

Loading

OakLoaf commented May 24, 2024 •

edited

Loading

BigTows commented May 24, 2024 •

edited

Loading

BigTows commented May 24, 2024 •

edited

Loading

OakLoaf commented May 24, 2024 •

edited

Loading

BigTows commented May 24, 2024 •

edited

Loading

BigTows commented May 25, 2024 •

edited

Loading

BigTows commented May 25, 2024 •

edited

Loading

OakLoaf commented May 25, 2024 •

edited

Loading

OakLoaf commented May 25, 2024 •

edited

Loading

BigTows commented May 25, 2024 •

edited

Loading

OakLoaf commented May 25, 2024 •

edited

Loading