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

JobsReborn Event Improvements #431

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

FireML
Copy link

@FireML FireML commented Jun 25, 2024

Adds <context.entity> and <context.block> to both the Jobs payment and EXP gain events.

Requested by myself: https://discord.com/channels/315163488085475337/1255295264072405032

@@ -72,6 +76,8 @@ public ObjectTag getContext(String name) {
case "job" -> job;
case "experience" -> new ElementTag(event.getExp());
case "action" -> new ElementTag(event.getActionInfo().getType().getName(), true);
case "entity" -> new EntityTag(event.getLivingEntity());
Copy link
Member

Choose a reason for hiding this comment

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

The docs say if applicable. but this code always returns an entity - one of the two is wrong

Copy link
Author

Choose a reason for hiding this comment

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

Yeah my bad. I've added a check for the entity being null 🙂

@@ -72,6 +76,8 @@ public ObjectTag getContext(String name) {
case "job" -> job;
case "experience" -> new ElementTag(event.getExp());
case "action" -> new ElementTag(event.getActionInfo().getType().getName(), true);
case "entity" -> event.getLivingEntity() == null ? null : new EntityTag(event.getLivingEntity());
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay, my laptop was out for 3 months so wasn't able to review PRs or anything sadly - assuming this entity could be a player/NPC, need to call #getDenizenObject on that EntityTag (same for the other event)

Copy link
Author

Choose a reason for hiding this comment

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

Done 🙂

Comment on lines 33 to 34
// <context.entity> Returns an EntityTag of the entity involved with this event, if applicable.
// <context.block> Returns a LocationTag of the block involved with this event, if applicable.
Copy link
Member

Choose a reason for hiding this comment

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

The meta here looks a little vague - what are the entity/block doing in the context of each event?

Copy link
Author

Choose a reason for hiding this comment

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

Jobs itself isn't very clear about this.

The entity is the target of the action, so if i killed a pig and earned money/exp, the entity would be the pig i killed.
It's similar for block, where the location is the block i would have modified.

I'm not really sure how else to word it in the meta.

Copy link
Author

Choose a reason for hiding this comment

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

Acting on feedback from discord, I've added an example and slightly changed the wording on the reviewed lines

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