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

ProjectKorra Bridge #425

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

Conversation

xicxen
Copy link

@xicxen xicxen commented Mar 30, 2024

Take 3:

pull request #424
pull request #423

lib modifications & extra contributor removed.


// <--[event]
// @Events
// projectkorra player|entity dies|death|killed by <ability>
Copy link
Member

Choose a reason for hiding this comment

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

This is a really awkward event format -- maybe do projectkorra <entity> dies with switch by:<ability>

// @Triggers when an entity dies from bending.
//
// @Context
// <context.ability> returns ElementTag(String) of the ability's name.
Copy link
Member

Choose a reason for hiding this comment

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

(String) is not a thing, and either way you don't need to specify types so heavily, just <context.ability> returns the ability's name.

@Override
public boolean matches(ScriptPath path) {
String ability = path.eventArgLowerAt(4);
// Check if event applies to any ability
Copy link
Member

Choose a reason for hiding this comment

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

Don't need comments that just say what the next line is doing (unless there's something unclear that needs explanation)

}

@EventHandler
public void onAbilityStart(EntityBendingDeathEvent event) {
Copy link
Member

Choose a reason for hiding this comment

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

copy/pasted method name

Removed types in @context

Removed comments

Fixed method name
@xicxen
Copy link
Author

xicxen commented Mar 30, 2024

Done


// <--[event]
// @Events
// projectkorra entity dies|death|killed
Copy link
Member

Choose a reason for hiding this comment

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

This should be <entity> ie dynamic match the entity

Copy link
Member

Choose a reason for hiding this comment

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

also the three dies/death/killed variants are silly, pick one


@Override
public boolean matches(ScriptPath path) {
if ((path.eventArgLowerAt(3).equals("by")) && (ability == null || !path.tryArgObject(4, ability))) {
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be here

removed variants

removed line 50 (and like from other events)
@xicxen
Copy link
Author

xicxen commented Mar 31, 2024

Done

@mcmonkey4eva
Copy link
Member

please don't touch the 'resolve' button

@xicxen
Copy link
Author

xicxen commented Mar 31, 2024

Understood

// -->

public EntityBendingDeathScriptEvent() {
registerCouldMatcher("projectkorra entity killed");
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 projectkorra <entity> killed has an entity matcher, but the code doesn't seem to - need to have the same syntax in here, and check for the entity matcher in matches (ScriptPath#tryArgObject)


@Override
public ScriptEntryData getScriptEntryData() {
return new BukkitScriptEntryData(PlayerTag.mirrorBukkitPlayer(event.getAbility().getPlayer()), null);
Copy link
Member

Choose a reason for hiding this comment

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

Can use the single-entity constructor, I.e. just pass in the player

@mcmonkey4eva
Copy link
Member

What's the status here? There's been unaddressed comments waiting for two months now

@xicxen
Copy link
Author

xicxen commented Jun 10, 2024

Apologies. I plan on working on them very soon.

I have had a family member pass away and have been preoccupied with other emergencies.

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