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

Dedicated primitive example #11697

Merged
merged 13 commits into from
Feb 14, 2024
Merged

Dedicated primitive example #11697

merged 13 commits into from
Feb 14, 2024

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Feb 4, 2024 •
edited
Loading

I just implemented this to record a video for the new blog post, but I figured it would also make a good dedicated example. This also allows us to remove a lot of code from the 2d/3d gizmo examples since it supersedes this portion of code.

Depends on: #11699

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 4, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 4, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@Jondolf Jondolf added C-Examples An addition or correction to our examples A-Math Fundamental domain-agnostic mathematical operations labels Feb 4, 2024
@RobWalt RobWalt mentioned this pull request Feb 4, 2024
Merged
@RobWalt RobWalt force-pushed the dedicated-primitive-example branch 2 times, most recently from 138fa9f to 879e2e5 Compare February 4, 2024 17:13
@RobWalt
Copy link
Contributor Author

RobWalt commented Feb 4, 2024

For anyone reviewing this: Please note that I extracted a bugfix which was previously included. The polygon in 2D will be fixed. The bugfix can be found here f7473ec

github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2024
@RobWalt @alice-i-cecile
fix(primitives): fix polygon gizmo rendering bug (#11699)
381f3d3 
This is just a minor fix extracted from #11697

A logic error. We tried to close the polygon shape, if the user
specifies an
unclosed polygon. The closing linestring previously didn't close the
polygon
though, but instead added a zero length line at the last coordinate.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
lynn-lumen pushed a commit to lynn-lumen/bevy that referenced this pull request Feb 5, 2024
@RobWalt @alice-i-cecile @lynn-lumen
fix(primitives): fix polygon gizmo rendering bug (bevyengine#11699)
ad24b15 
This is just a minor fix extracted from bevyengine#11697

A logic error. We tried to close the polygon shape, if the user
specifies an
unclosed polygon. The closing linestring previously didn't close the
polygon
though, but instead added a zero length line at the last coordinate.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Jondolf
Jondolf suggested changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Typos and naming nits

examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
@Jondolf
Copy link
Contributor

Jondolf commented Feb 5, 2024

I like the look of the actual example, but:

  • The naming doesn't quite work when using the same enum for 2D and 3D, e.g. this is not a circle:

not a circle

  • The 2D view doesn't have shape names: (the layout is also a bit different)

no names

  • There are several empty choices in the 2D mode without explanation

empty

tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
@RobWalt @alice-i-cecile @tjamaan
fix(primitives): fix polygon gizmo rendering bug (bevyengine#11699)
5a078c4 
This is just a minor fix extracted from bevyengine#11697

A logic error. We tried to close the polygon shape, if the user
specifies an
unclosed polygon. The closing linestring previously didn't close the
polygon
though, but instead added a zero length line at the last coordinate.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@RobWalt
Copy link
Contributor Author

RobWalt commented Feb 6, 2024

I like the look of the actual example, but:

* The naming doesn't quite work when using the same enum for 2D and 3D, e.g. this is not a circle:

not a circle

* The 2D view doesn't have shape names: (the layout is also a bit different)

no names

* There are several empty choices in the 2D mode without explanation

empty

I'm not really sure how to do this (yet?). Somehow it's hard for me to position the text consistently between 2D and 3D views. On my screen it looked ok but I see that the current state won't work for everyone. If you have any ideas on how to resolve that, let me know.

@RobWalt
Copy link
Contributor Author

RobWalt commented Feb 6, 2024

@Jondolf just addressed the last issue in my lunch break 👍🏼

Jondolf
Jondolf suggested changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

There's a bug with text not updating (see review comment below) and a few more nits, but otherwise the example looks good now!

It could be nice to show the active dimension with text too, but I won't block over that

examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
examples/math/primitives.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Jondolf
Jondolf approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

File name should be snake-cased, but otherwise LGTM

Cargo.toml Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

2 similar comments
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@Jondolf
Copy link
Contributor

Jondolf commented Feb 12, 2024

Meshing for most 3D primitives was added in #11688, so this example should probably be updated to reflect that

@RobWalt
Copy link
Contributor Author

RobWalt commented Feb 13, 2024

@Jondolf done 👍🏼

@RobWalt
docs(examples): add a dedicated example for primitives
85506ba 
This should also help users to check which primitives have rendering
support already.

Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
chore(cleanup): fix warnings in new example
7185961 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
chore(cleanup): remove primitives code from gizmo examples
e3ecfb0 
superseded by dedicated example

Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
chore(cleanup): do what CI told me to do 👀
e1557ed 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
fix(ci): no bevy_internal imports
9e699eb 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
docs(instruction): add instructions for users
26d89ad 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
chore(cleanup): rename last -> previous
b356f77 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
fix(review): implement all review suggestions
c547cb9 
Authored-by: RobWalt <robwalter96@gmail.com>
Reviewed-by: Joona Aalto <jondolf.dev@gmail.com>
@RobWalt
fix(ui): text is now properly displayed in 2D and 3D
7a37e6c 
The issue was that, by default, only one camera renders the UI. This was
defaulted to the 3D camera we spawned. A recently merged PR added the
functionality to change which Camera UI Nodes are rendered on via a
component. (`TargetCamera`)

We use that here and it works. Thanks to @teodosin on discord helping me
to find this out <3

Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
fix(reviewed): fix typos, naming and bug
7d00ea1 
Authored-by: RobWalt <robwalter96@gmail.com>
Reviewed-by: Joona Aalto <jondolf.dev@gmail.com>
@RobWalt
fix(filenames): 🐍
c3af01f 
Authored-by: Aviac <aviac@mailbox.org>
@RobWalt
chore(cleanup): regenerate the examples
930b6ac 
Authored-by: RobWalt <robwalter96@gmail.com>
@RobWalt
feat(meshes-dim3): add 3D meshes to example
d773e15 
Authored-by: Aviac <aviac@mailbox.org>
@alice-i-cecile alice-i-cecile mentioned this pull request Feb 14, 2024
Merged
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 14, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 14, 2024
Merged via the queue into bevyengine:main with commit b446374 Feb 14, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpecificProtagonist SpecificProtagonist SpecificProtagonist left review comments

@Jondolf Jondolf Jondolf approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RobWalt @Jondolf @alice-i-cecile @SpecificProtagonist