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

Feat: [#3157] return target on trigger #3176

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

Autsider666
Copy link
Contributor

@Autsider666 Autsider666 commented Aug 23, 2024

===:clipboard: PR Checklist :clipboard:===

  • 📌 issue exists in github for these changes
  • 🔬 existing tests still pass
  • 🙈 code conforms to the style guide
  • 📐 new tests written and passing / old tests updated with new scenario(s)
  • 📄 changelog entry added (or not needed)

==================

Couldn't get the suggested setup to work without adding a lot of extra (code) complexity. I'd recommend dropping that requirement, because it's a lot easier to implement by the developers themselves by adding the check in their own filter callback to assure an entity is of a specific type (and use as .... to placate TS if needed).

I'll finish the last 2 points on the checklist after there's a green light on my current implementation.

Closes #3157

Changes:

  • The action callback of Trigger now returns the entity that triggered it.
  • General cleanup of Trigger class
    • target is now 100% separate from filter, because that's what the functionality described in the documentation implied (to me).
    • Converted all the different "default value fallbacks" to a single setup.
    • Trigger used Entity and Actor interchangeably. Rewrote it to be 100% Entity (Although Entity doesn't have any collision mechanics build in, so an argument could be made to go for 100% Actor)

@Autsider666
Copy link
Contributor Author

Ah, those tests have to be fixed.
Is there any magic ritual to get the sandbox command to run locally? I was hope sacrificing a chicken was enough, but it'll take a bit to find a goat...
image

@eonarheim
Copy link
Member

Ah, those tests have to be fixed. Is there any magic ritual to get the sandbox command to run locally? I was hope sacrificing a chicken was enough, but it'll take a bit to find a goat... image

There indeed is some hidden magic apologies, I've been meaning to fix up the sandbox to a modern build. I've got a half completed stale branch (https://github.com/excaliburjs/Excalibur/tree/chore/improve-sandbox-qol)

The gist is you copy the built artifacts into the sandbox lib folder then run the build

  1. Build ex umd module npm run core:bundle
  2. Copy ex to lib and build sandbox npm run sandbox:copy && npm run sandbox:build

@eonarheim
Copy link
Member

eonarheim commented Aug 28, 2024

@Autsider666 there are some flakey unit tests on main I haven't nailed down yet either :/

Copy link
Member

@eonarheim eonarheim left a comment

Choose a reason for hiding this comment

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

@Autsider666 looks great!

@Autsider666 Autsider666 force-pushed the feat/return-target-on-trigger branch from 38dd008 to f02ef89 Compare August 29, 2024 08:03
@Autsider666
Copy link
Contributor Author

@eonarheim Updated the changelog, so PR is ready to be merged

@eonarheim eonarheim merged commit 29e8317 into excaliburjs:main Aug 29, 2024
3 checks passed
@eonarheim
Copy link
Member

@Autsider666 Many thanks!

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.

feat: Trigger - access to action's actor
2 participants