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.

Sign up for GitHub

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

Jump to bottom

👌 Use Offset type directly in JoystickAction.update calculations #631

Merged
merged 2 commits into from
Jan 18, 2021
Merged

👌 Use Offset type directly in JoystickAction.update calculations #631

merged 2 commits into from
Jan 18, 2021

Conversation

lig
Copy link
Contributor

@lig lig commented Jan 15, 2021 •
edited
Loading

Description

Improved code a bit to utilize Offset support for + and - operators.

Fixes #606

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

If something is unclear, please submit the PR anyways and ask about what you thought was unclear.

  • This branch is based on the latest master
  • I have formatted my code with flutter format
  • The continuous integration (CI) is passing
  • Changelog entry

@lig
Copy link
Contributor Author

lig commented Jan 15, 2021 •
edited
Loading

The original suggestion by @luanpotter was to use Vector2 type. It's turned out that using Offset produces less code still allowing direct calculations with it.

spydon
spydon approved these changes Jan 16, 2021
View reviewed changes
Copy link
Member

@spydon spydon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, much easier to read too!

Wouldn't it be the same amount of code qith Vector2 though? 🤔
We are trying to use Vector2 as much as possible for these datatypes.

Also, please add a changelog line.

@lig
Copy link
Contributor Author

lig commented Jan 16, 2021 •
edited
Loading

Wouldn't it be the same amount of code qith Vector2 though? 🤔
We are trying to use Vector2 as much as possible for these datatypes.

The resulting value of the expression needs to be converted to Offset to pass it further. nextPoint we are controlling fully here. However, other values are Offsets. Using Vector2 results in 3 type conversions: to Vector2 for two input values and converting the result back to Offset.
I believe that it's better to avoid conversions as much as possible. For Vector2 to be adopted more widely the values should be created as Vector2 in the first place.

@lig
Copy link
Contributor Author

lig commented Jan 16, 2021 •
edited
Loading

Also, please add a changelog line.

Ok, I wasn't able to come up with anything meaningful for this change;) but I'll figure something out.

@spydon
Copy link
Member

spydon commented Jan 16, 2021

Makes sense, and I agree

erickzanardo
erickzanardo requested changes Jan 17, 2021
View reviewed changes
Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

LGTM, only missing the changelog entry, gonna mark a request changes on the PR so we don't merge this before having the entry

luanpotter
luanpotter approved these changes Jan 17, 2021
View reviewed changes
Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

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

LGTM

@lig lig force-pushed the 606-improve-joystick-action-update-logic branch from 8eb41a3 to 7b884f9 Compare January 17, 2021 22:21
@lig lig requested a review from erickzanardo January 17, 2021 22:22
@erickzanardo erickzanardo merged commit 4e63c31 into flame-engine:master Jan 18, 2021
@lig lig deleted the 606-improve-joystick-action-update-logic branch January 18, 2021 06:57
renancaraujo pushed a commit that referenced this pull request Jan 19, 2021
@lig
👌 Use Offset type directly in JoystickAction.update calculations (#…
867dcc5 
…631)
renancaraujo pushed a commit that referenced this pull request Jan 19, 2021
@lig
👌 Use Offset type directly in JoystickAction.update calculations (#…
85c6a11 
…631)
erickzanardo added a commit that referenced this pull request Jan 20, 2021
@renancaraujo @lig @erickzanardo
Move files to src and comply with the dart package layout convention (#…
00ad86e 
…621)

* 👌 Use `Offset` type directly in `JoystickAction.update` calculations (#631)

* Move files to src and comply with the dart package layout convention

* Fixing widgets example

Co-authored-by: Serge Matveenko <lig@countzero.co>
Co-authored-by: Erick Zanardo <erickzanardoo@gmail.com>
spydon added a commit that referenced this pull request Feb 24, 2021
@lig @spydon
@wolfenrain @erickzanardo @renancaraujo
null safety (#549)
0420830 
* Update README to point at rc5 (#630)

* Update README to point at rc5

* Update README.md

* 👌 Throw exception from `parseAnchor` in `examples/widgets` instead of returning null (#632)

* Updated the widgets docs (#628)

Co-authored-by: Erick <erickzanardoo@gmail.com>

* Removing initialDimensions and removeGesture methods (#627)

* Removing initialDimensions and removeGesture methods

* Removing wrongly commited folder

* PR suggestion

* Making SpriteComponent and SpriteAnimationComponent follow the same standard for empty constructors (#620)

* Added fallback support for the web on the `SpriteBatch` API (#612)

* Added fallback support for the web on the `SpriteBatch` API

* Refactored the SpriteBatch class

* 👌 Use `Offset` type directly in `JoystickAction.update` calculations (#631)

* Move files to src and comply with the dart package layout convention (#621)

* 👌 Use `Offset` type directly in `JoystickAction.update` calculations (#631)

* Move files to src and comply with the dart package layout convention

* Fixing widgets example

Co-authored-by: Serge Matveenko <lig@countzero.co>
Co-authored-by: Erick Zanardo <erickzanardoo@gmail.com>

* Fix gesture detection on children (#636)

* Fix gesture detection on children

* Remove unused imports

* positionParent should be private

* Fixed comment

* Moved parent to BaseComponent

* Fix formatting

* Initial implementation of the Composition class. (#634)

* Initial implementation of the Composition class

* Added decodeImageFromPixels web support

* Added a `required` to GameWidget and updated the docs (#638)

* Added a required to GameWidget and updated the docs

* Removed entry from CHANGELOG

* Release v1.0.0-rc6 (#639)

* Add hitbox to PositionComponent (#618)

* Move out collision detection methods

* Add possibility to define a hull for PositionComponents

* Add example of how to use hull with tapable

* Update contains point comment

* Fix contains point

* Hull should be based on center position

* Remove collision detection parts

* Added tests

* Use percentage of size instead of absolute size

* Separate hull from PositionComponent

* Clarify hull example

* Fix formatting

* Override correct method

* Use mixin for hitbox

* Update changelog

* Rename HasHitbox to Hitbox

* Clarified names

* Center to edge is considered as 1.0

* Fix test

* Add spaces within braces

* Removed extra spaces in the braces

* Add hitbox docs

* Fix link

* Moved point rotation to Vector2 extension

* Render hitbox within extension

* Fix rebase

* Fix rebase

* Fix formatting

* Removing Util.dart and moving its remaining parts to better places (#640)

* Removing Util.dart and moving its remaining parts to better places

* Fixing lint

* Doc fixes

* Doc fixes

* PR suggestions

* Adapting SpriteBatchComponent constructors to other components (#643)

* Adapting ParallaxComponent constructors to other components (#642)

* Adapating ParallaxComponent constructors to other components

* Removing wrongly commited folder

* Update doc/components.md

Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>

* Update doc/components.md

Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>

Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>

* Updating mds with recently 0.x patches

* 🏷 Null safety support

Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>
Co-authored-by: Erick <erickzanardoo@gmail.com>
Co-authored-by: Renan <6718144+renancaraujo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfenrain wolfenrain wolfenrain approved these changes

@renancaraujo renancaraujo renancaraujo approved these changes

@luanpotter luanpotter luanpotter approved these changes

@erickzanardo erickzanardo erickzanardo approved these changes

@spydon spydon spydon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve JoystickAction.update logic
6 participants
@lig @spydon @erickzanardo @luanpotter @renancaraujo @wolfenrain