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: YSortComponent #3057

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: YSortComponent #3057

wants to merge 3 commits into from

Conversation

mattjennings
Copy link
Contributor

===: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)

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

Changes:

  • YSortComponent that adjusts z-index based on its global Y position. This can be used on an
    Actor with the new ySort configuration in the constructor, or added as a component to an Entity.

    new ex.Actor({
      ySort: true,
      // or
      ySort: {
        offset: 0, // apply an offset to the resulting z-index
        order: 1 // 1 will add the pos.y to the z-index, -1 will subtract it
      }
    });
    
    // or
    entity.addComponent(
      new ex.YSortComponent({
        offset: 0,
        order: 1
      })
    );
CleanShot.2024-05-11.at.18.09.35.mp4

Open to any feedback or suggestions on functionality / API.

Copy link

cloudflare-workers-and-pages bot commented May 11, 2024

Deploying excaliburjs with  Cloudflare Pages  Cloudflare Pages

Latest commit: e4b60d1
Status: ✅  Deploy successful!
Preview URL: https://d8f368ca.excaliburjs.pages.dev
Branch Preview URL: https://feat-ysort.excaliburjs.pages.dev

View logs

@github-actions github-actions bot added the enhancement Label applied to enhancements or improvements to existing features label May 11, 2024
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.

@mattjennings Looks completely reasonable to me, no notes! Let me know if you're ready to merge

@eonarheim
Copy link
Member

We spoke offline, we are going to test this with some Tilemap use cases first before merging

@mattjennings
Copy link
Contributor Author

After discussion on discord, we decided:

  • Scene should have a config that will enable ysorting for all entities.
  • YSortComponent should take optional min/max z values as a way to enable layering.

Further ideas from myself since discussion:

  • The Scene config should only apply to entities that contain a YSortComponent
  • YSortComponent is included in all actors by default, and uses the Scene's ySort config unless overridden. In other words, if it's enabled by Scene, then every Actor sorts.
  • Caveat: would mean we need to add YSortComponent to tilemap plugins etc. May be easy to miss it when composing custom entities. Maybe the YSort logic should be in the TransformComponent?
class MyScene extends ex.Scene {
  constructor() {
      super({ 
        ySort: true,
        // and/or ?
       ySort: { enabled: true, ...otherConfig }
      })
   }
}

class MyActor {
   constructor() {
      super({
          // ySort config defaults to scene's ySort
      })
   }
}

Copy link

This PR hasn't had any recent activity lately and is being marked as stale automatically.

@github-actions github-actions bot added the stale This issue or PR has not had any activity recently label Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Label applied to enhancements or improvements to existing features stale This issue or PR has not had any activity recently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants