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 running Turtle logic in the browser via WebAssembly #53

Closed
wants to merge 46 commits into from

Conversation

Projects
None yet
2 participants
@marshallpierce
Copy link

commented Dec 31, 2017

This introduces a new Runtime type. This encapsulates all the platform-specific parts of running Turtle logic: communication between turtle logic and rendering backend, how grahpics are actually rendered, PRNG, timekeeping, etc. DesktopRuntime simply uses the existing code, rearranged a little bit so that the desktop-specific bits are in desktop (e.g. Server now has from_piston_event, update_window and the render loop from Renderer since those are strongly tied to piston-window things).

  • Implementations of Runtime for both canvas and desktop
  • Implementation of Piston's Graphics that writes to a RGBA pixel buffer as needed by a <canvas>.
  • Move desktop-only code to desktop module, with analogous canvas module
  • Animation now uses the clock available from Runtime
  • Rendering is now generic on Graphics so that it can work with either desktop or canvas flavors
  • Turtle is now a newtype for GenericTurtle so that all the types work out, but users don't need to care about the generic in practice.
  • Change entry point to use a macro instead of main(). A Turtle is initialized differently for desktop vs wasm use, so we need to hide that from users. Also, webassembly has to provide some parameters when it passes control flow to user logic, so it can't just re-use main or something like that. This also prevents users from ever doing startup wrong.
  • Expose random number generation on Turtle type so that users will always have a safe PRNG accessible without worrying about platform issues. Until Rust gets NLL in stable, this requires an extra local var in certain uses (e.g. turtle.set_color(turtle.random::<Color>()) doesn't work yet without extracting a local for the color).

Todo:

  • Change documentation to match. I didn't go to the trouble until we have more clarity about what the API will look like.

N.B. I think the names RenderProcess and Server aren't as helpful as they could be for guiding a reader. The RenderProcess is the thing that isn't the render process, for instance: it's more of a client library for working with the other process. Server might be called RenderWorkerProcess or something like that; it's not really serving anything. Anyway, now that startup is done via a macro, the fork model could be moved back to a multithreaded model easily enough and then the whole issue goes away.

marshallpierce added some commits Dec 9, 2017

Introduce RenderStrategy trait.
Push all Piston-specific code behind an implementation of that trait.
Get things compiling with wasm32.
Use lower level piston crates as needed to avoid platform dependencies.
Pull render loop out of Renderer into Server.
This allows Renderer to only use platform-agnostic piston primitives.
Remove re-exported random that depends on thread_rng and expose random
on Turtle instead so it can use an appropriate Rng.
Fix the first few doc tests to not create a Turtle.
Using the run_turtle! macro required nesting the macro invocation
inside a `fn main()` so that rust doc didn't create its own main().
This would never be done in practice, but since the tests are
just testing that the code compiles, it's good enough for this.
@@ -74,7 +74,7 @@ fn dragon(
if num_folds == 0 {
// mapping a color number 0-255 to an rgb gradient.
turtle.set_pen_color(Color {
red: (color_mid - 128.).abs() * 2.,
red: (color_mid - 127.5).abs() * 2.,

This comment has been minimized.

Copy link
@marshallpierce

marshallpierce Dec 31, 2017

Author

this avoids a crash as the recently added color checking logic does its thing

@@ -90,3 +40,53 @@ impl DerefMut for Maze {
&mut self.grid
}
}

/// Generates a new random maze
pub fn generate<R: Rng>(rng: &mut R) -> Maze {

This comment has been minimized.

Copy link
@marshallpierce

marshallpierce Dec 31, 2017

Author

The function is generic and outside the struct so that the struct need not be generic on Rng.

@sunjay

This comment has been minimized.

Copy link
Owner

commented Jan 1, 2018

Thanks for opening this pull request! I am attempting to restart the errored jobs that probably shouldn't have failed but Travis is currently giving me some issues. I'll try again later.

I am unfortunately quite busy this week and this weekend, but I will try to get back to this the week of the 8th. Really appreciate all the work you've put into this!

@sunjay

This comment has been minimized.

Copy link
Owner

commented Jan 14, 2018

@marshallpierce My apologies for making you wait for so long. I unfortunately have a couple of things I need to get to in both turtle and my personal life before I can give this PR more attention. I don't have an exact timeline yet, but please know that I do have this mind and will get to it as soon as possible in the coming weeks. :)

@sunjay

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2018

Hi @marshallpierce, I'd like to personally take responsibility for the stagnation of this PR. Looking back, I should have asked you to make smaller incremental changes that are faster to review. Taking the time out to review a huge set of changes like this really hasn't been easy. Going back and forth making adjustments will take a lot of time too.

Since that is totally on me, and I don't expect you to go back and break down your changes into small increments (that's a lot of work), I'll do everything I can to do the work for you. I'll bring in your changes slowly (using your commits so the work is still tagged with your name) and keep as much of it as I can.

Here's what's been on my to-do list that's been preventing me from looking at this:

  • Finish off the last few things in the 1.0.0 milestone
  • Release first 1.0.0 release candidate
  • Write the guide
  • Release 1.0.0 (assuming there are no major issues)

This is quite a lot and I was hoping to finish it sooner, but life has gotten in the way. This WebAssembly work would fit in there around right after I finish writing the guide and before releasing 1.0.0 since that is the point where I would really have confidence that the API is good and stable.

Anyway, like I said, this doesn't really require anymore work from your end. Once I'm ready and I've finished those other things I need to work on, I'll come back and address all of this.

Thanks again for your work! The time you spent figuring out how to do this is very valued and appreciated! Sorry I didn't do a good enough job of steering you in a direction that would have helped get this merged sooner.

@marshallpierce

This comment has been minimized.

Copy link
Author

commented Feb 28, 2018

OK, I'm glad it won't die. :)

@sunjay

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2018

@marshallpierce Thanks again for all your work on this! I learned a lot from this experience and like I said, I'll be making sure that I am more responsible about getting more manageable PRs in the future. So sorry that this wasn't a more positive experience for you!

Have an excellent weekend! :)

@sunjay sunjay closed this Jul 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.