ocean reef
Another interesting issue I just found. I'm working on creating a villager NPC with my customized code. I turned targetable for NPC by accident and then a zombie started attacking it.
But when I typed targetable command again the zombie still keep attacking the NPC. I'll had to despawn it and then respawn it so zombie will lose target.
Is this work as expected?
Log there: https://paste.denizenscript.com/View/127631
NPC saves: https://paste.denizenscript.com/View/127632
void sirenBOT
(snwcreations) targetable command can't stop NPCs from being targeted
void sirenBOT
Another interesting issue I just found. I'm working on creating a villager NPC w...
Hi I'm AutoThreadBot! Don't mind me, I'll just be adding the helper team to this thread so they can see it. A human will get to you soon.
marble isle
Another interesting issue I just found. I'm working on creating a villager NPC w...
I guess it doesn't cancel any existing targets. Problem is that to cancel existing targets you need to iterate through nearby entities
ocean reef
I think it is possible to track entities targeting it.
marble isle
Oh you want to track? Hmm might be a lot of memory requirements
I'd prefer to iterate nearby entities
ocean reef
I just think it is weird so I made this post, if it is not a bug then I can also accept it as it does not affect my project
marble isle
I just think it is weird so I made this post, if it is not a bug then I can also...
No we can try and cancel targeting entities that's fine
south gulchBOT
ocean reef
Oh you want to track? Hmm might be a lot of memory requirements
I think it will not cost so many memories so it is possible
We can just track UUIDs and then search them again while we need to stop them from targeting
If they are unloaded then that's good as they are no longer able to target entities
And this could be adapted by adding an option and marking it a workaround so users could choice if they really need this fix
ocean reef
I'd prefer to iterate nearby entities
The opinion I forget to say is that nearby entities method may not be able to cover all entities targeting it as the distance from them to it is unclear
some mobs have longer track distance but some is not
So that is why I suggest tracking by us because we can do this by listening on target events
and tracking events will be better while there is a lot of entities around NPC as only entities which want to target NPC will fire an event
I'm just unsure if the target events is available since 1.8 for compat (as this thought is unnecessary but should consider)
okay it is available even since 1.7.10
ocean reef
I've written an initial patch for it and now im testing it
I'll make a PR if it succeeded but still have something to discuss
As I don't know if entities which is being unloaded will have EntityTargetEvent with null targets fired
good, my patch works
Unfortunately I had stored Mob instances to a list so I could retrieve it when we need to clear targets, but I don't want memory leaks caused by unloaded Mob instances so I was just thinking about if there is some way I could do graceful cleanups
So any ideas? I'm making PR.
PR made as 3171, here we go: https://github.com/CitizensDev/Citizens2/pull/3171
Oh unused import inside but I'll clean it when I'm available as I'm closing my computer and go outside
I'll keep Discord online as I had nothing to do but I could only chat on my phone for now
marble isle
@ocean reef I'll do a proper GitHub review in an hour or so but I think the key should go into npc.metadata enum or maybe even a trait, and use Bukkit.getEntity(uuid) to get the mob
ocean reef
Unfortuantely there is no way to retrieve entities through World instance with only UUIDs
marble isle
Unfortuantely there is no way to retrieve entities through World instance with o...
The Bukkit class has a method from memory
Well I guess Server
ocean reef
Oh I don't know that. I'll check it on my schedule.
Oh cool it exists
I'll edit it. scheduled.
ocean reef
<@807184045414154261> I'll do a proper GitHub review in an hour or so but I thin...
yes so more changes on it is needed as it is just an initial version.
marble isle
I wonder if it makes more sense as a trait then its self contained
ocean reef
I agree it should be self contained
But I don't think it should be a trait
a metadata is ok
as it is never updated by itself but only outside events
ocean reef
ah I'm free now
Moved the PR logic towards UUID-based storage but still wanted to know where should we put the storage, I think the metadata key I use should not be exposed as it is implementation-specific thing (IDK will someone implement Citizens API by himself)
and it could be just a metadata
marble isle
Moved the PR logic towards UUID-based storage but still wanted to know where sho...
that's why it could be trait, it can still be in metadata though
ocean reef
uh also fine as trait can have hard-typed objects
okay I'll turn it into a trait. wait a min
done
marble isle
@ocean reef how does PlayerMoveEvent work?
ocean reef
<@807184045414154261> how does PlayerMoveEvent work?
I've checked that and it is fired by packet handler
upbeat iglooBOT
I've checked that and it is fired by packet handler
ocean reef
When accepting movement packets it got fired
ocean reef
looking at paper source
yes but it is fired by CB impl of Player.teleport
as plugins can pass custom reasons to it
a bit unrelated to this, I'll rename the added trait if we had discussed a proper name for it
ocean reef
a bit unrelated to this, I'll rename the added trait if we had discussed a prope...
done and can continue
and I was thinking that user devs may also need to do clearTargets manually if they changes the targetable metadata, should we consider a solution for this? May we do this for them automatically or just keep it as-is?
marble isle
and I was thinking that user devs may also need to do clearTargets manually if t...
you can add the api if you want
ocean reef
I prefer the first one but I want to keep direct changes to metadata also function as what I did in this PR so I'll need something as callback
But I had no idea on designing that
it will be tricky to add if (metadata == TARGETABLE && !newValue) to setMetadata methods
marble isle
it will be tricky to add `if (metadata == TARGETABLE && !newValue)` to setMetada...
not sure what this means
ocean reef
that is what I did in /npc targetable
if we had changed the targetable status to false then I'll call clearTargets there
But I want to inline this call into setMetadata
but it is not a good design as it breaks principle of single responsibility in software designing
so still thinking about alternatives
or I'll give it up
actually I currently had no idea on this so I need help
ocean reef
fine I give it up
but I'll leave this open as I think it is not an end for this but I'll have to sleep for now
marble isle
can make it all a trait if you want
ocean reef
okay I get up after a short rest. and I think you're right. so I think it is early to merge that PR as doing so will need to rename the trait again, I'm willing to rename it to TargetableTrait and add other logic related to it, but I'm afraid I'll break userdev plugins as I regard the main module as an API (should I drop this opinion?)
and also I want to make the changes to targetable metadata be eqivalent to the change to targetable trait and then deprecate the metadata.
We may do this by a per-tick basis in trait but may not reflect changes from metadata value immediately as it will have a 1 tick delay.
ocean reef
did that but not fully solved yet, as I don't know if the userdevs will still want to use targetable metadata and how they modify that, persist or not?
uh really tough
marble isle
Sorry i made some cleanup changes which I need to push first
marble isle
is there any conflict?
Yes
ocean reef
fine, feel free to push and I'll handle it
marble isle
@ocean reef I pushed just now
ocean reef
pulling
uh totally make the PR broken, I'll have to restart the patch
done sync
ready to continue discussion
ocean reef
so the only issue I had no idea is that IDK how check if a NPC metadata value is persist, as I don't have access to MetadataValue.persist field
because of that I can't handle legacy calls gracefully
marble isle
@ocean reef hmm, although I like temporary metadata, just make it all persistent for now
ocean reef
what I think is that targetable trait only save the targetable state but also targeters temporarily
marble isle
what I think is that targetable trait only save the targetable state but also ta...
targeters should also be temporary always
ocean reef
yes and I hope my codes on it are right
ocean reef
<@807184045414154261> hmm, although I like temporary metadata, just make it all ...
may confuse old userdevs
as I hope their code works as before so that's why I still thinking on that
I really cares backwards compat
marble isle
@ocean reef I care as well, but it's just been a new minecraft and citizens version so now is a good time
ocean reef
you mean just drop that, even without deprecating that metadata for some time first?
marble isle
you mean just drop that, even without deprecating that metadata for some time fi...
yes
we don't have as many API users to be that extreme with API breakage
ocean reef
okay... I'll remove the legacy handler for now
done, so PR is out of draft state now
the targetable metadata enum should be removed once the PR is merged
and after that this thread could be closed
ocean reef
<@807184045414154261> hmm, although I like temporary metadata, just make it all ...
it is noticeable that the trait also supports temporary state switch, I just can't handle the legacy way for temporary targetable state changes, but we don't need to discuss it anymore as we've decided to drop that
umm you may need to do cleanup again for that as IDK why IntelliJ sorts imports in such a way
ocean reef
PR was merged but it is out of initial state so I'll close this thread after doing test on it
