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

docs: Added documentation for GameLoop class #1234

Merged
merged 11 commits into from
Jan 18, 2022

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Dec 16, 2021

Description

Added documentation for the GameLoop class.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, doc: 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

  • No, this is not a breaking change.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder if there was a specific reason to have the separate methods, since they weren't doing exactly the same thing.

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.

Docs LGTM, but this PR is addressing multiple things under the same PR, I don't understood the need to refactor pause and resume, but this should be treated on a separate PR.

packages/flame/lib/src/game/game_loop.dart Outdated Show resolved Hide resolved
@st-pasha
Copy link
Contributor Author

I don't understood the need to refactor pause and resume

As I was writing the documentation for each method in the class, I found that it's hard to describe what those methods do as compared to start and stop. I mean, sure they had their own quirks, but it's better to fix those quirks than document them.

_ticker.muted = true;
previous = Duration.zero;
}
@Deprecated('Internal variable')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use Deprecated? There are specialized decorators for this like Protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we don't use that variable anywhere. It was never intended to be public.
So, now that we make it private, the getter is provided in case anyone's code was reading that variable. The getter is marked deprecated, as a way to signal that this variable really shouldn't be used, and that the getter will be removed after a while.

@st-pasha st-pasha marked this pull request as draft December 27, 2021 00:47
@st-pasha st-pasha marked this pull request as ready for review January 17, 2022 06:27
@spydon spydon merged commit b1d4e58 into flame-engine:main Jan 18, 2022
@st-pasha st-pasha deleted the ps/game-loop branch January 18, 2022 21:35
spydon added a commit that referenced this pull request Mar 12, 2023
…rs (#2395)

<!--
The title of your PR on the line above should start with a [Conventional
Commit] prefix
(`fix:`, `feat:`, `docs:`, `test:`, `chore:`, `refactor:`, `perf:`,
`build:`, `ci:`,
`style:`, `revert:`). This title will later become an entry in the
[CHANGELOG], so please
make sure that it summarizes the PR adequately.
-->

# Description
<!--
Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior,
how this PR is changing it,
and what motivated the change. If this is a breaking change, specify
explicitly which APIs were
changed.
-->

@eukleshnin identified issues with building the documents locally on a
Windows workstation. Namely, the following was occurring:

```
reading sources... [  2%] flame/layout/align_component
Exception occurred:

FileNotFoundError: [WinError 2]
```
This was determined to be a combination of several things.

1. In `dart_domain.py`, the `subprocess.run` calling dartdoc_json did
not have the shell parameter set to true. This solves the error about
the file not being found. This then generates errors that subsequent
references to the `temp_file` did not exist.
2. This was due to the default setting with Python `tempfile` where when
it determines the temp file has been closed, it deletes it; however, it
was still needed, so by setting the `delete=False` parameter, the file
would still remain.
3. Unfortunately, because it still remains, it needs to be deleted once
it is no longer needed. Trying to use `finally:` with the `try` block
failed to produce the results desired, so the temp file name was
registered with the class so it can be deleted in the original calling
function if it exists. This proved successful.
4. Although not critical, the same temp file uses a suffix of `json` so
it was creating files `xxxxjson`. By adding the "." in the suffix, it
creates valid file names now. This doesn't actually fix anything, it
just seemed wrong, so I fixed it to be valid files if ever needed down
the road.

Now that the docs built, there were several warnings that could be
resolved:
1. `Overlays.md` was not referenced in the TOC tree.
2. Since overlays were removed in
#2384, the Platformer tutorial
had a link to the old path and it needed to be updated.

Finally and open for discussion, during this process of debugging, I
upgraded all packages to the most current to see what impacts there
were. The following is the old and new potential `requirements.txt`:

```
----Old
linkify-it-py==2.0.0
myst-parser==0.18.1
Pygments==2.12.0
Sphinx==5.0.2
sphinxcontrib-mermaid==0.8.1
sphinx-autobuild==2021.3.14
jinja2==3.1.2

----- New
linkify-it-py==2.0.0
myst-parser==1.0.0
Pygments==2.14.0
Sphinx==6.1.3
sphinxcontrib-mermaid==0.8.1
sphinx-autobuild==2021.3.14
Jinja2==3.1.2
```

The only byproduct of this upgrade was a deprecated package warning for
`attrs_image` in `conf.py` which was updated to `attrs_inline`. I made
that change initially for this PR, but backed it out as I didn't know if
there was a desire to update the `requirements.txt` and felt some
discussion may be warranted.

Regardless, with everything upgraded or left as is, the other fixes
resolve the issues on Windows.

## Checklist
<!--
Before you create this PR confirm that it meets all requirements listed
below by checking the
relevant checkboxes with `[x]`. If some checkbox is not applicable, mark
it as `[-]`.
-->

- [X] I have followed the [Contributor Guide] when preparing my PR.
- [ ] I have updated/added tests for ALL new/updated/fixed
functionality.
- [X] I have updated/added relevant documentation in `docs` and added
dartdoc comments with `///`.
- [ ] I have updated/added relevant examples in `examples` or `docs`.


## Breaking Change?
<!--
Would your PR require Flame users to update their apps following your
change?

If yes, then the title of the PR should include "!" (for example,
`feat!:`, `fix!:`). See
[Conventional Commit] for details. Also, for a breaking PR uncomment and
fill in the "Migration
instructions" section below.

### Migration instructions

If the PR is breaking, uncomment this header and add instructions for
how to migrate from the
currently released version to the new proposed way.
-->

- [ ] Yes, this PR is a breaking change.
- [X] No, this PR is not a breaking change.


## Related Issues
<!--
Indicate which issues this PR resolves, if any. For example:

Closes #1234
!-->

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md

---------

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
eukleshnin pushed a commit to eukleshnin/flame that referenced this pull request Mar 12, 2023
…rs (flame-engine#2395)

<!--
The title of your PR on the line above should start with a [Conventional
Commit] prefix
(`fix:`, `feat:`, `docs:`, `test:`, `chore:`, `refactor:`, `perf:`,
`build:`, `ci:`,
`style:`, `revert:`). This title will later become an entry in the
[CHANGELOG], so please
make sure that it summarizes the PR adequately.
-->

# Description
<!--
Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior,
how this PR is changing it,
and what motivated the change. If this is a breaking change, specify
explicitly which APIs were
changed.
-->

@eukleshnin identified issues with building the documents locally on a
Windows workstation. Namely, the following was occurring:

```
reading sources... [  2%] flame/layout/align_component
Exception occurred:

FileNotFoundError: [WinError 2]
```
This was determined to be a combination of several things.

1. In `dart_domain.py`, the `subprocess.run` calling dartdoc_json did
not have the shell parameter set to true. This solves the error about
the file not being found. This then generates errors that subsequent
references to the `temp_file` did not exist.
2. This was due to the default setting with Python `tempfile` where when
it determines the temp file has been closed, it deletes it; however, it
was still needed, so by setting the `delete=False` parameter, the file
would still remain.
3. Unfortunately, because it still remains, it needs to be deleted once
it is no longer needed. Trying to use `finally:` with the `try` block
failed to produce the results desired, so the temp file name was
registered with the class so it can be deleted in the original calling
function if it exists. This proved successful.
4. Although not critical, the same temp file uses a suffix of `json` so
it was creating files `xxxxjson`. By adding the "." in the suffix, it
creates valid file names now. This doesn't actually fix anything, it
just seemed wrong, so I fixed it to be valid files if ever needed down
the road.

Now that the docs built, there were several warnings that could be
resolved:
1. `Overlays.md` was not referenced in the TOC tree.
2. Since overlays were removed in
flame-engine#2384, the Platformer tutorial
had a link to the old path and it needed to be updated.

Finally and open for discussion, during this process of debugging, I
upgraded all packages to the most current to see what impacts there
were. The following is the old and new potential `requirements.txt`:

```
----Old
linkify-it-py==2.0.0
myst-parser==0.18.1
Pygments==2.12.0
Sphinx==5.0.2
sphinxcontrib-mermaid==0.8.1
sphinx-autobuild==2021.3.14
jinja2==3.1.2

----- New
linkify-it-py==2.0.0
myst-parser==1.0.0
Pygments==2.14.0
Sphinx==6.1.3
sphinxcontrib-mermaid==0.8.1
sphinx-autobuild==2021.3.14
Jinja2==3.1.2
```

The only byproduct of this upgrade was a deprecated package warning for
`attrs_image` in `conf.py` which was updated to `attrs_inline`. I made
that change initially for this PR, but backed it out as I didn't know if
there was a desire to update the `requirements.txt` and felt some
discussion may be warranted.

Regardless, with everything upgraded or left as is, the other fixes
resolve the issues on Windows.

## Checklist
<!--
Before you create this PR confirm that it meets all requirements listed
below by checking the
relevant checkboxes with `[x]`. If some checkbox is not applicable, mark
it as `[-]`.
-->

- [X] I have followed the [Contributor Guide] when preparing my PR.
- [ ] I have updated/added tests for ALL new/updated/fixed
functionality.
- [X] I have updated/added relevant documentation in `docs` and added
dartdoc comments with `///`.
- [ ] I have updated/added relevant examples in `examples` or `docs`.


## Breaking Change?
<!--
Would your PR require Flame users to update their apps following your
change?

If yes, then the title of the PR should include "!" (for example,
`feat!:`, `fix!:`). See
[Conventional Commit] for details. Also, for a breaking PR uncomment and
fill in the "Migration
instructions" section below.

### Migration instructions

If the PR is breaking, uncomment this header and add instructions for
how to migrate from the
currently released version to the new proposed way.
-->

- [ ] Yes, this PR is a breaking change.
- [X] No, this PR is not a breaking change.


## Related Issues
<!--
Indicate which issues this PR resolves, if any. For example:

Closes flame-engine#1234
!-->

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md

---------

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
spydon pushed a commit that referenced this pull request Apr 10, 2023
fixing link to bare flame tutorial

<!--
The title of your PR on the line above should start with a [Conventional
Commit] prefix
(`fix:`, `feat:`, `docs:`, `test:`, `chore:`, `refactor:`, `perf:`,
`build:`, `ci:`,
`style:`, `revert:`). This title will later become an entry in the
[CHANGELOG], so please
make sure that it summarizes the PR adequately.
-->

# Description
<!--
Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior,
how this PR is changing it,
and what motivated the change. If this is a breaking change, specify
explicitly which APIs were
changed.
-->


## Checklist
<!--
Before you create this PR confirm that it meets all requirements listed
below by checking the
relevant checkboxes with `[x]`. If some checkbox is not applicable, mark
it as `[-]`.
-->

- [ ] I have followed the [Contributor Guide] when preparing my PR.
- [ ] 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` or `docs`.


## Breaking Change?
<!--
Would your PR require Flame users to update their apps following your
change?

If yes, then the title of the PR should include "!" (for example,
`feat!:`, `fix!:`). See
[Conventional Commit] for details. Also, for a breaking PR uncomment and
fill in the "Migration
instructions" section below.

### Migration instructions

If the PR is breaking, uncomment this header and add instructions for
how to migrate from the
currently released version to the new proposed way.
-->

- [ ] Yes, this PR is a breaking change.
- [x] No, this PR is not a breaking change.


## Related Issues
<!--
Indicate which issues this PR resolves, if any. For example:

Closes #1234
!-->

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md
erickzanardo added a commit that referenced this pull request Sep 19, 2023
…meAudio (#2751)

<!--
The title of your PR on the line above should start with a [Conventional
Commit] prefix
(`fix:`, `feat:`, `docs:`, `test:`, `chore:`, `refactor:`, `perf:`,
`build:`, `ci:`,
`style:`, `revert:`). This title will later become an entry in the
[CHANGELOG], so please
make sure that it summarizes the PR adequately.
-->

# Description
<!--
Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior,
how this PR is changing it,
and what motivated the change. If this is a breaking change, specify
explicitly which APIs were
changed.
-->

It was already possible to update the prefixes of FlameAudio assets, but
it required a certain understanding of `FlameAudio` and `Audioplayers`.

This PR adds a simple helper method that makes it easier to change
without requiring much context of the internals of the package.

## Checklist
<!--
Before you create this PR confirm that it meets all requirements listed
below by checking the
relevant checkboxes with `[x]`. If some checkbox is not applicable, mark
it as `[-]`.
-->

- [x] I have followed the [Contributor Guide] when preparing my PR.
- [x] 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` or `docs`.


## Breaking Change?
<!--
Would your PR require Flame users to update their apps following your
change?

If yes, then the title of the PR should include "!" (for example,
`feat!:`, `fix!:`). See
[Conventional Commit] for details. Also, for a breaking PR uncomment and
fill in the "Migration
instructions" section below.
-->

- [ ] Yes, this PR is a breaking change.
- [x] No, this PR is not a breaking change.

<!--
### Migration instructions

If the PR is breaking, uncomment this header and add instructions for
how to migrate from the
currently released version to the new proposed way.
-->

## Related Issues
<!--
Indicate which issues this PR resolves, if any. For example:

Closes #1234
!-->

<!-- Links -->
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Conventional Commit]: https://conventionalcommits.org
[CHANGELOG]:
https://github.com/flame-engine/flame/blob/main/CHANGELOG.md
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.

4 participants