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

Testing documentation examples #99

Merged
merged 44 commits into from
Jun 3, 2024

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented May 23, 2024

Testing all examples is very time consuming, and checking them all by hand is error prone. Having an easy way to test them would help.

What this PR does:

  • create a new workspace docs-examples, containing Rust examples (I didn't start to work on javascript)
    • We can easily test that all examples compile through cargo check --examples
  • For the templates: defines a new <load> tag (in the mindset of <bevy>, <rust>, <js>...
    • the new workspace docs-examples also contains a crate inject_code, which is responsible to parse a file, detect the load tag, and inject the code inside, keeping only relevant marked lines.
    • a new folder templates_injected is created, containing all the templates with injected code,in order to play well with PUBLISH_MODE=0, so we can review templates on their aggregated page.
    • another folder inject_file/tmp_diff is created, containing diffs from template to injected files. Mostly for debug purposes, not sure if we keep it, I might turn this behaviour off by default
  • the ./generate_user_guides.sh script is updated to do the injection automatically.
Solution considerations

It's a bit unfortunate that it creates an indirection when editing the docs (we have to go check the content of the file linked), but the alternatives being:

  • 🔴 Don't change anything, and risk errors at each new releases
  • 🔴 Inverse the source of truth: create a rust project from the documentation. It's a bit more complicated to do, and risks having very big files for the doc

Migration fixes or modification details (long)

  • colliders:
    • collider type modification: sensor component added/removed
    • colliderposition: z position moved in 2d examples ; missing rparenthesis ; missing rangle
    • colliders solver/group flags ( "Collision groups and solver groups" for bevy plugin is out of date #75 )
    • ActiveCollisionTypes::KINEMATIC_FIXED -> KINEMATIC_STATIC
      • ⚠️ : TODO: rename STATIC to fixed in bevy_rapier
    • ColliderFlagsComponent -> ActiveHooks
    • RigidBody without GlobalTransform, with child ; causing runtime warning.
      • I added GlobalTransform
    • Quat::IDENTIY -> Quat::IDENTITY
  • joints:
    • local_anchor -> local_anchor1
    • added colliders + SpatialBundle, maybe too verbose but more useful.
    • Vector3::x_axis() -> Vec3::X (use bevy rather than na)
    • useless mut 🤷
  • CharacterController:
    • That collider optionally be attached to a rigid-body
    • character_controller -> character_controllers
    • to_radians() needs explicit type
    • modify_character_controller_autostep -> modify_character_controller_up
    • modify_character_controller_slopes -> read_character_controller_collisions
  • Scene queries:
  • advanced collision detection
    • collide handle -> ColliderHandle
    • sample code to add the marker ActiveEvents::CONTACT_FORCE_EVENTS ?
    • ⚠️ : check solo lines codes shared with rust/js, in bevy anyway: has_any_active_contact -> has_any_active_contacts
    • manifold data -> .raw.data + direct properties (not function calls)
    • contacts_with -> contact_pairs_with
    • intersections_with -> intersection_pairs_with
    • App::build -> App::new

Bugs:

  • ⚠️ : pixels_per_meter doesn't work ?
  • ⚠️ : CharacterController broken (with bevy)
  • grounded becomes sometimes false (Strange KinematicCharacterControllerOutput behaviour bevy_rapier#377)
  • update translation must be in FixedUpdate
  • examples lack a SpatialBundle or TransformBundle
  • doesn't work without a collider ? (maybe just needs a shape?)
  • 2 characterController on top of one another end up blocked.
  • solver_contacts ref needed for borrow checker + function is actually a property.

Tutorial flow notes

My feeling over going through the bevy user guide is quite positive, as it's very precise, and useful to make sense of how it works together.

It might be bit more resembling to a reference than a "guide" though, with a vibe of "it will all make sense when you finish, promise".

Suggestions:

  • avoid code snippet which are `too isolated, prefer slightly more lines of code but usefulness.

Also, there's a lot of code shared withing this user guide, we might want to consider consolidating the crates documentation for reference, and link to those rather than re-implementing and maintaining our own rustdoc ?

Or study if we can apply some of ideas from https://diataxis.fr/

@Vrixyz Vrixyz force-pushed the fix_doc_examples_code_test branch from fbf87d7 to e5c689c Compare May 23, 2024 09:05
@@ -0,0 +1 @@
v16.20.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not certain it's useful, but I wasn't sure which node version to use through yarn

println!("Ball altitude: {}", transform.translation.y);
}
}
// DOCUSAURUS: stop basic_sim
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for simplicity I chose to always have markers start/stop, even if we inject the whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: now its // DOCUSAURUS: basic_sim stop

docs-examples/3d/bevy/examples/rigid_bodies3.rs Outdated Show resolved Hide resolved
docs-examples/2d/bevy/Cargo.lock Outdated Show resolved Hide resolved
yarn.lock Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I wrongly ran npm update, this should probably be reverted


cp -r docs/user_guides/templates/* docs/user_guides/rust/.
cp -r docs/user_guides/templates/* docs/user_guides/bevy_plugin/.
cp -r docs/user_guides/templates/* docs/user_guides/javascript/.

find docs/user_guides/bevy_plugin/ -type f -print0 | xargs -0 gawk -i inplace -v RS='<rapier>[^<]*</rapier>' -v ORS= '1'
find docs/user_guides/bevy_plugin/ -type f -print0 | xargs -0 gsed -i '/<rapier>/,/<\/rapier>/d'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these should be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still I'm curious if it's needed ; I tested from windows and mac and I didn't notice any major difference (but maybe the devil is in the details?)

Copy link
Member

@sebcrozet sebcrozet May 31, 2024

Choose a reason for hiding this comment

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

Let’s keep your changes. Maybe there is something weird going on my end.

package.json Outdated Show resolved Hide resolved
Comment on lines 66 to 67
// Trimming end for cross platform, on windows I had \r finishing result.
assert_eq!(result.trim_end(), "correct data1 1");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious about an explaination as to why I'm getting "\r" ; I believe my source file only has "\n" 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced all \r\n with \n ; I assume that won't be aproblem 🤷

docs-examples/inject_file/src/main.rs Outdated Show resolved Hide resolved
docs-examples/inject_file/src/main.rs Show resolved Hide resolved
inject_code_in_user_guides.sh Outdated Show resolved Hide resolved
inject_code_in_user_guides.sh Outdated Show resolved Hide resolved
@Vrixyz Vrixyz marked this pull request as ready for review May 30, 2024 13:59
@Vrixyz Vrixyz force-pushed the fix_doc_examples_code_test branch from c97db84 to 284b9d3 Compare May 30, 2024 14:02
@Vrixyz Vrixyz force-pushed the fix_doc_examples_code_test branch from 284b9d3 to 08801ef Compare May 30, 2024 14:10
inject_code_in_user_guides.sh Outdated Show resolved Hide resolved
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thank you!

  • Would it make sense if the <load ... /> substitution also added the '''rust backtics automatically?
  • Could we rename <load ... /> into <example ... />?


cp -r docs/user_guides/templates/* docs/user_guides/rust/.
cp -r docs/user_guides/templates/* docs/user_guides/bevy_plugin/.
cp -r docs/user_guides/templates/* docs/user_guides/javascript/.

find docs/user_guides/bevy_plugin/ -type f -print0 | xargs -0 gawk -i inplace -v RS='<rapier>[^<]*</rapier>' -v ORS= '1'
find docs/user_guides/bevy_plugin/ -type f -print0 | xargs -0 gsed -i '/<rapier>/,/<\/rapier>/d'
Copy link
Member

@sebcrozet sebcrozet May 31, 2024

Choose a reason for hiding this comment

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

Let’s keep your changes. Maybe there is something weird going on my end.

Comment on lines 10 to 12
cp -r docs/user_guides/templates/* docs/user_guides/rust/.
cp -r docs/user_guides/templates/* docs/user_guides/bevy_plugin/.
cp -r docs/user_guides/templates/* docs/user_guides/javascript/.
Copy link
Contributor Author

@Vrixyz Vrixyz May 31, 2024

Choose a reason for hiding this comment

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

should load injected_templates ?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Jun 3, 2024

  • Would it make sense if the <load ... /> substitution also added the '''rust backtics automatically?

I decided against because the substitution is language agnostics, I'll want to use the same code for javascript injections. It also allows to add other code snippet if needs be. I'm not feeling too opinionated in this though.

  • Could we rename <load ... /> into <example ... />?

I have no strong opinion on this either ; this injection could work to load another markdown file though (it's probably not a good idea as there are better tools), but I felt "load" conveys better its inner working. Alternatives considered were "file", "inject"...

@Vrixyz Vrixyz requested a review from sebcrozet June 3, 2024 11:37
@Vrixyz Vrixyz merged commit 160f573 into dimforge:master Jun 3, 2024
@Vrixyz Vrixyz mentioned this pull request Jun 4, 2024
for mut transform in positions.iter_mut() {
dbg!(transform.rotation.to_axis_angle());
transform.rotation = Quat::from_rotation_z(270_f32.to_radians());
//println!("Ball altitude: {}", transform.translation.y);
Copy link

Choose a reason for hiding this comment

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

@Vrixyz this doesn't seem to print ball altitude anymore.

I suspect that debug code was put here then forgotten about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! not sure what went wrong about this indeed :o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 participants