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

Support wrapping ex.Text graphics #2472

Merged
merged 52 commits into from
Jan 14, 2023
Merged

Conversation

RyanGrieb
Copy link
Contributor

@RyanGrieb RyanGrieb commented Aug 27, 2022

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

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

Closes #2463

Changes:

  • Add maxWidth TextOption to support word wrapping

Hi, I think I've progressed enough to post a draft PR.
Currently wrapping works when the maxWidth option is provided for the ex.Text class.

There is a bug currently where text > 2 lines isn't rendered since I don't update the ex.Text height properly when we word wrap.
If there is any other outstanding problems in the code/PR please let me know!

Screenshot_2022-08-27_15-08-35

@eonarheim
Copy link
Member

@RyanGrieb thanks for the PR! I'll have some time to look this over this week sometime👍

@eonarheim
Copy link
Member

@RyanGrieb the code looks good to my eyes! Exactly what I would have expected

There is one failing test, I'm trying to figure out why the text generated is slightly shifted up and to the left. It might be the case that the new changes are more correct, I'm just not sure why it's different quite yet.

image

@eonarheim
Copy link
Member

@RyanGrieb Actually! I believe the PR code is more correct the resetTransform() prevents the context transform from being corrupted with previous calls to _applyFont() which is a problem in the current main branch. Many thanks!

I think this PR is ready, update the failing test, add a new test for the maxWidth, and fix the height issue you spoke about 👍

@RyanGrieb
Copy link
Contributor Author

@eonarheim I believe corrected the height issue. I'm not sure how you'd like me to correct the failing test. Should I replace the image? Also, how would I individually run the test 722 and visually look at it.
image
Looking good 😄
Once I fix the failing test I'll complete the rest of the PR checklist & merge the separate commit messages.

@RyanGrieb
Copy link
Contributor Author

Also I haven't tested this w/ ex.SpriteFont. I don't think we should merge until I look at it & test it properly.

@eonarheim
Copy link
Member

I'm not sure how you'd like me to correct the failing test. Should I replace the image? Also, how would I individually run the test 722 and visually look at it.

@RyanGrieb good questions!

Yes please do replace the expected test images if you can (you can copy paste the base64 image into a browser and download it to the appropriate spec in src/spec/images folder). You can grab the expected linux image from the failing test in the github action, but if you don't have a windows machine I can add a commit to your fork with expected windows image 👍

You can add an f to the front of the test it to "focus" the test and only run that one fit('can reuse a font', async () => {.

Really appreciate the work on this feature! It'll be a huge value add to Excalibur!

@RyanGrieb
Copy link
Contributor Author

Word wrap for sprite font added, add changelog entry
image
Weird spacing on the left side though, not sure if it's supposed to be like this.

@eonarheim
Copy link
Member

@RyanGrieb This is looking great! Let me know when you're ready for review

Weird spacing on the left side though, not sure if it's supposed to be like this.

@RyanGrieb I think that's expected with the SpriteFont by default in the current implementation. The SpriteFont image has some extra padding in the individual sprites per glyph. Although there may be a case for adjusting the first letter offset in addition to specifying the spacing, but I think that would be best in a separate issue/PR 👍

image

eonarheim and others added 6 commits September 11, 2022 18:30
…2410)

Closes excaliburjs#2401

This PR implements an arbitrary raycasting api in Excalibur

```typescript
const hits = engine.currentScene.physics.rayCast(new ex.Ray(player.pos, ex.Vector.Down), {
  maxDistance: 100,
  collisionGroup: blockGroup,
  searchAllColliders: false
});
```

<img width="797" alt="image" src="https://user-images.githubusercontent.com/612071/178625499-d9f97052-5b18-49fe-9440-89bd93e46ff6.png">

## Changes:

- Fix issue where TileMaps weren't correctly added to the collider data structure
- Add physics world with raycasting endpoint
@github-actions
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 Nov 11, 2022
renovate bot and others added 15 commits December 1, 2022 08:49
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…2534)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…js#2533)

Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2.
- [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases)
- [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2)

---
updated-dependencies:
- dependency-name: decode-uri-component
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [loader-utils](https://github.com/webpack/loader-utils) from 1.1.0 to 1.4.2.
- [Release notes](https://github.com/webpack/loader-utils/releases)
- [Changelog](https://github.com/webpack/loader-utils/blob/v1.4.2/CHANGELOG.md)
- [Commits](webpack/loader-utils@v1.1.0...v1.4.2)

---
updated-dependencies:
- dependency-name: loader-utils
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@eonarheim
Copy link
Member

Hi @RyanGrieb sorry about the delay on this PR! Life happened to me!

Is it okay if I push a few commits to your fork? This is definitely something that would be nice to include in the next release of excalibur!

@eonarheim eonarheim removed the stale This issue or PR has not had any activity recently label Dec 3, 2022
@RyanGrieb
Copy link
Contributor Author

@eonarheim Go for it. I couldn’t get tests working for some reason on my machine.

@eonarheim eonarheim marked this pull request as ready for review December 3, 2022 03:54
@eonarheim eonarheim merged commit 51ea101 into excaliburjs:main Jan 14, 2023
@eonarheim
Copy link
Member

@RyanGrieb Thanks for working on this feature! Sorry about the long delay on the merge!

We appreciate the hard work this is great feature!

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.

Support wrapping ex.Text graphics
3 participants