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!: Added getImageLayer to flame_tiled #1405

Merged
merged 17 commits into from
Feb 27, 2022
Merged

feat!: Added getImageLayer to flame_tiled #1405

merged 17 commits into from
Feb 27, 2022

Conversation

munsterlander
Copy link
Contributor

@munsterlander munsterlander commented Feb 26, 2022

Description

This is a very simple add to flame_tile to allow getting image layers.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the
relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
  • I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

Does your PR require Flame users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change. (Indicate it in the [Conventional Commit] prefix with a !,
    e.g. feat!:, fix!:).
  • No, this is not a breaking change.

Copy link
Member

@ufrshubham ufrshubham left a comment

Choose a reason for hiding this comment

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

I was thinking should we make it a generic method

T getLayer<T extends Layer>(String name){
    final t = map.layers.firstWhere((layer) {
      return layer is T && layer.name == name;
    });
    return t as T;
}

@munsterlander
Copy link
Contributor Author

Which I concur with and if @spydon wants to implement this, then we can go in and change all the docs, examples, and tests. It is a breaking change though, so I don't know if a phased approach is better, i.e. have all 5 methods co-exist and mark the specific implementations as deprecated (even though 3 would be new), have just 2 methods and mark the old as deprecated with the generic one there, or just break it and only have 1.

@munsterlander
Copy link
Contributor Author

munsterlander commented Feb 26, 2022

Documenting implementation in case there is a question:

getLayer<ObjectGroup>("myObjectGroupLayer");
getLayer<ImageLayer>("myImageLayer");
getLayer<TileLayer>("myTileLayer");
getLayer<Group>("myGroupLayer");

Copy link
Contributor

@st-pasha st-pasha left a comment

Choose a reason for hiding this comment

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

  1. Looks like the three functions can be combined into a single template function.
  2. Please use doc-comments (///) for documentation. You may read more about them in this guide.
  3. Consider what would happen when the map doesn't have the requested layer.
  4. Perhaps add a few tests?

@munsterlander
Copy link
Contributor Author

@st-pasha Yes, ideally it can return null to avoid errors with calls to non-existent layers. Ok, I will update this pr with the relevant change. Do we need to leave the existing method and just label it as deprecated?

@st-pasha
Copy link
Contributor

Do we need to leave the existing method and just label it as deprecated?

yes, breaking changes should be avoided if possible. So, leaving the existing method as-is and marking it as @Deprecated() is the right idea.

@spydon
Copy link
Member

spydon commented Feb 26, 2022

Looks good, I agree with @ufrshubham that it would be nice as a generics method, and with @st-pasha regarding the test, deprecation and docs.

@munsterlander munsterlander changed the title feat: Added getImageLayer to flame_tiled feat!: Added getImageLayer to flame_tiled Feb 26, 2022
@munsterlander
Copy link
Contributor Author

munsterlander commented Feb 26, 2022

Ok, I think that does it. We added the code, tests, examples, doc block, and docs. I updated the PR to indicate blocking and remove unnecessary comments. I also have pulled in the upstream changes.

@spydon spydon enabled auto-merge (squash) February 27, 2022 16:42
@spydon spydon merged commit a037ada into flame-engine:main Feb 27, 2022
st-pasha pushed a commit to st-pasha/flame that referenced this pull request Feb 27, 2022
erickzanardo added a commit that referenced this pull request Feb 28, 2022
* position_type_test

* detectors_test

* reformat projections_test

* Reorganize tests in projector_test

* review flame_game_test

* move some cameratests

* created viewport_test file

* reformat camera tests

* feat: Add missing optional priority to SpriteBodyComponent (#1404)

* Add missing optional priority to SpriteBodyComponent

Gives you the option to set the priority directly when creating the component.

* Add optional parameter priority

Adds priority as an optional parameter

* removed wrong trailing comma

Added a comma at the wrong position.

* feat:  Added getImageLayer to flame_tiled (#1405)

* feat: Create sphinx extension for integrating Flutter apps into the documentation site (#1393)

Co-authored-by: KurtLa <KurtLa@users.noreply.github.com>
Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com>
Co-authored-by: Erick <erickzanardoo@gmail.com>
st-pasha pushed a commit to st-pasha/flame that referenced this pull request Mar 9, 2022
st-pasha added a commit to st-pasha/flame that referenced this pull request Mar 9, 2022
* position_type_test

* detectors_test

* reformat projections_test

* Reorganize tests in projector_test

* review flame_game_test

* move some cameratests

* created viewport_test file

* reformat camera tests

* feat: Add missing optional priority to SpriteBodyComponent (flame-engine#1404)

* Add missing optional priority to SpriteBodyComponent

Gives you the option to set the priority directly when creating the component.

* Add optional parameter priority

Adds priority as an optional parameter

* removed wrong trailing comma

Added a comma at the wrong position.

* feat:  Added getImageLayer to flame_tiled (flame-engine#1405)

* feat: Create sphinx extension for integrating Flutter apps into the documentation site (flame-engine#1393)

Co-authored-by: KurtLa <KurtLa@users.noreply.github.com>
Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com>
Co-authored-by: Erick <erickzanardoo@gmail.com>
spydon added a commit that referenced this pull request Mar 9, 2022
* ProcessQueues() method

* Added deadQueue in LifecycleManager

* refactor removeFromParent()

* make shouldRemove non-overridable

* Added lifecycle state "removing"

* prevent double-removal

* shouldRemove can only be set to true

* eliminate _shouldRemove

* rename _dead -> _dying

* added adoption queue

* removed nextParent

* deprecate ComponentSet.clear and .removeAll

* ComponentSet no longer handles removal

* remove usages of shouldRemove

* doc-comments

* onRemove refactor

* cleanup

* remove obsolete paragraph in docs

* rename queue dying -> removals

* feat: update examples dashbook (#1398)

* docs: Added tutorial for creating a bare Flame project (#1376)

* feat: Add missing optional priority to SpriteBodyComponent (#1404)

* Add missing optional priority to SpriteBodyComponent

Gives you the option to set the priority directly when creating the component.

* Add optional parameter priority

Adds priority as an optional parameter

* removed wrong trailing comma

Added a comma at the wrong position.

* feat:  Added getImageLayer to flame_tiled (#1405)

* feat: Create sphinx extension for integrating Flutter apps into the documentation site (#1393)

* feat: adding FlameBloc mixin to allow its usage with enhanced FlameGame classes (#1399)

* feat: adding FlameBloc mixin to allow its usage with enhanced FlameGame classes

* fixing tests

* Apply suggestions from code review

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
Co-authored-by: Pasha Stetsenko <stpasha@google.com>
Co-authored-by: Luan Nico <luanpotter27@gmail.com>

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
Co-authored-by: Pasha Stetsenko <stpasha@google.com>
Co-authored-by: Luan Nico <luanpotter27@gmail.com>

* refactor: Organize tests in the game/ folder (#1403)

* position_type_test

* detectors_test

* reformat projections_test

* Reorganize tests in projector_test

* review flame_game_test

* move some cameratests

* created viewport_test file

* reformat camera tests

* feat: Add missing optional priority to SpriteBodyComponent (#1404)

* Add missing optional priority to SpriteBodyComponent

Gives you the option to set the priority directly when creating the component.

* Add optional parameter priority

Adds priority as an optional parameter

* removed wrong trailing comma

Added a comma at the wrong position.

* feat:  Added getImageLayer to flame_tiled (#1405)

* feat: Create sphinx extension for integrating Flutter apps into the documentation site (#1393)

Co-authored-by: KurtLa <KurtLa@users.noreply.github.com>
Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com>
Co-authored-by: Erick <erickzanardoo@gmail.com>

* feat: improving generics on position body component (#1397)

* chore(release): publish packages (#1407)

- flame@1.1.0-releasecandidate.1
 - flame_bloc@1.2.0-releasecandidate.1
 - flame_rive@1.1.0-releasecandidate.1
 - flame_test@1.2.0-releasecandidate.1
 - flame_tiled@1.3.0-releasecandidate.1

* chore: fixing pub deps to allow publish (#1408)

* chore(release): publish packages

 - flame@1.1.0-releasecandidate.1
 - flame_bloc@1.2.0-releasecandidate.1
 - flame_rive@1.1.0-releasecandidate.1
 - flame_test@1.2.0-releasecandidate.1
 - flame_tiled@1.3.0-releasecandidate.1

* chore: fixing deps to enable pub publish

* fixing vector math version

* chore(flame_forge2d): export all files in barrel file (#1409)

* chore(release): publish packages (#1410)

- flame_forge2d@0.9.0-releasecandidate.1

* chore: commented out PR template sections (#1412)

* feat: Make ContactCallback begin end methods optional overrides (#1415)

* feat: made begin and end optional overrides

* chore: removed unecessary end override

* feat: Camera as a component (#1355)

* feat(collision detection)!: Use a broadphase to make collision detection more efficient (#1252)

* fix: PositionBodyComponent had an async onMount, without needing (#1424)

* fix: Fix collision detection comments and typo (#1422)

* Fix collision detection comments and typo

* Update packages/flame/lib/src/collisions/collision_callbacks.dart

Co-authored-by: Pasha Stetsenko <stpasha@google.com>

* Update doc/flame/collision_detection.md

Co-authored-by: Pasha Stetsenko <stpasha@google.com>

Co-authored-by: Pasha Stetsenko <stpasha@google.com>

* feat: adding has mounted to component (#1418)

* feat: adding has mounted to component

* feat: pr suggestions

* feat: improving hasMounted

* feat: renaming hasMounted to mounted

* feat: pr suggestion

* chore(release): publish packages (#1427)

- flame_svg@1.1.0-releasecandidate.1
 - flame@1.1.0-releasecandidate.2
 - flame_bloc@1.2.0-releasecandidate.2
 - flame_forge2d@0.9.0-releasecandidate.2
 - flame_rive@1.1.0-releasecandidate.2
 - flame_test@1.2.0-releasecandidate.2
 - flame_tiled@1.3.0-releasecandidate.2

* fix a test

* fix broken merge

* rename parent->owner in LifecycleManager

* update doc-comment

Co-authored-by: Erick <erickzanardoo@gmail.com>
Co-authored-by: KurtLa <KurtLa@users.noreply.github.com>
Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com>
Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
Co-authored-by: Luan Nico <luanpotter27@gmail.com>
Co-authored-by: Allison Ryan <77211884+allisonryan0002@users.noreply.github.com>
Co-authored-by: Alejandro Santiago <dev@alestiago.com>
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.

5 participants