Skip to content
This repository has been archived by the owner on Apr 18, 2022. It is now read-only.

Port amethyst_controls to legion. #2325

Closed
wants to merge 9 commits into from

Conversation

dkushner
Copy link
Collaborator

@dkushner dkushner commented Jun 22, 2020

Description

This PR continues work to integrate Legion ECS into Amethyst by porting the amethyst_controls module.

Modifications

  • Moved all existing systems definitions in amethyst_controls from struct-based to function-based using the Legion system builder.

PR Checklist

By placing an x in the boxes I certify that I have:

  • Updated the content of the book if this PR would make the book outdated.
  • Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
  • Added unit tests for new code added in this PR.
  • Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

  • Ran cargo +stable fmt --all
  • Ran cargo clippy --all --features "empty"
  • Ran cargo test --all --features "empty"

@AlveLarsson AlveLarsson linked an issue Jun 23, 2020 that may be closed by this pull request
@AnneKitsune
Copy link
Contributor

how is it going? do you need help on something? :3

@dkushner dkushner marked this pull request as ready for review July 4, 2020 02:32
@dkushner
Copy link
Collaborator Author

dkushner commented Jul 4, 2020

I'm opening this up for review to get some help/feedback on the implementation so far. I waffled between wanting to change up the entire system structure and sticking as close to the original implementation as possible. In the end, I tried to stick as close to the original implementation so that I could iteratively improve functionality. Things I need to fix:

  • Free movement system should apply the entity's rotation before translating for proper camera-relative movement.
  • Remove accumulated roll from free rotation system.
  • Figure out why mouse look input and movement input are not executed simultaneously.

There seems to be a number of places where the current implementation diverges with the "Legion way" of doing things, and I'm hoping that somebody can advise me on a more idiomatic way of accomplishing this.

@khionu
Copy link
Member

khionu commented Jul 7, 2020

Please note that a handful of minor bugs shouldn't be hard blockers. We would be happy to review lingering issues to see if we'd be happy to merge as is. While we would like to avoid regressions, this is part of a major change, and we can always try to fix them between merge and release.

@@ -19,30 +19,31 @@ license = "MIT/Apache-2.0"
travis-ci = { repository = "amethyst/amethyst", branch = "master" }

[features]
default = ["animation", "audio", "locale", "network", "renderer"]
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert the changes to this file before the merge happens :)

@@ -66,6 +66,10 @@ pub fn build_udp_network_send_system(
delivery
),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll need to pull the most recent version of legion_v2 and do a merge

This was referenced Jul 20, 2020
@AnneKitsune
Copy link
Contributor

superseded by #2378

@CleanCut CleanCut added this to In progress in Legion integration via automation Nov 15, 2020
@CleanCut CleanCut moved this from In progress to Done in Legion integration Nov 15, 2020
@CleanCut CleanCut mentioned this pull request Nov 15, 2020
23 tasks
@CleanCut CleanCut added this to the 0.16.0 - Legion milestone Nov 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

amethyst_controls, complete port
4 participants