leopard-js / leopard

Library for making Scratch-like projects with JavaScript.
https://leopardjs.com
MIT License
145 stars 27 forks source link

Add Sprite.dragging ("set drag mode to") + reflect Scratch "click" behavior #201

Open towerofnix opened 4 months ago

towerofnix commented 4 months ago

Resolves #123.

Adds support for player-style sprite dragging. (Scratch has two drag styles; they're mainly visually different, e.g. so that you can drag a sprite right out of the stage and into your backpack. We only consider player-style dragging relevant for now.)

Draggability is enabled for a sprite by setting this.draggable = true, which is analogous to "set drag mode to (draggable)". Sprite initialization properties can also specify draggable: true.

Behavior is entirely modeled off of Scratch. We match these things:

You should review all the above behavior to confirm it's working as expected. A review looking for missed edge/corner cases would also be appreciated. We're slightly concerned about the lifetime of a clone, but we believe it should at least be completely discarded as soon as the mouse is released (and immediately, if it was actively being dragged).

(*) Original behavior was as follows:

  • Instead of grabbing the sprite under the mouse at the time of a started drag, we grab the sprite under the mouse at the time it was pressed down. This is subtly different. It should result in nicer drags (you can mouse-down over the very edge of a sprite, move the mouse away, and pick up the mouse you clicked). But, it may result in huge drag offsets if the sprite moves away before the drag actually begins.

^ Apart from differing from Scratch being the wrong choice by default (even in a "nice" way), the drag offset problem suggests this isn't worth it or would require more thought. We'll match Scratch behavior here before requesting review.

(**) Oh, but it gets dumber. Scratch checks to start a drag every frame (as long as no sprite is currently being dragged). If a draggable sprite enters where your drag started, then that is what gets picked up, regardless where your mouse is now, when you perform a mousemove and you've met the "far enough from where I started" condition. Same behavior if the drag-start was attempted due to the 400ms one-off timeout: doesn't matter where your mouse is now, the attempt is based on where you pressed the mouse down.

This means you can - in Scratch, and Leopard too - start a drag based on where you pressed down the mouse, despite your mouse now being lord knows where-else. Demo below!

https://github.com/leopard-js/leopard/assets/9948030/10d3e1b1-8946-4af7-b44e-a207232b596a

PullJosh commented 4 months ago

That video is FASCINATING. What a cool find. Thanks for putting so much effort into matching Scratch's behavior.

Like I said earlier, I'm away for a few days (typing this on my phone) so I can't review until later.

towerofnix commented 4 months ago

Thanks @PullJosh! 🥳 It blew us away too. Just such a funny little thing.

This pull request is now ready for review. We touched the code up with improved in-file behavior documentation, moved the listeners into separate functions, and gave it a general review for logic smell, so it should be pretty good to review. Please let us know when reviewing if anything is confusing or difficult to wrap your head around. (And no rush, of course!)

towerofnix commented 4 months ago

We also (of course) still need to file the sb-edit pull request. Not getting to that tonight 😅 We need to make sure the new property names (mainly draggable) are not used by generated functions etc.

Unfortunately, I don't think there's getting around breaking existing projects which have defined custom blocks (i.e. methods of the sprite subclass) called draggable. Um... oops? At least this is less impactful than if variables were stored right on the sprite?

PullJosh commented 4 months ago

That's a good point. Technically this is a breaking change because people are extending Sprite. Most Leopard projects import the most recent 1.x.x version of Leopard from unpkg and use that. Do we need to create a new major version? I think if we're being extremely principled about it the answer is yes, but I don't know what choice would actually be best for users in general.

towerofnix commented 4 months ago

Yes, bumping the major version is appropriate where Leopard depends on behavior/names that will collide the ways users have already extended their Sprite subclasses.

Of course the most ideal approach is to just not collide with those, but since the syntax we desire counts on adding a new property (draggable), there's no getting around it.

In the future we could use symbols as a completely alternate approach, to avoid unexpected collisions:

// in Leopard's code
class Sprite {
  ...
  static draggable = Symbol.for('Sprite.draggable');
  ...
}

// in your code, e.g. generated code
class MySprite extends Sprite {
  myScript*() {
    ...
    this[Sprite.draggable] = true;
    ...
  }
}

Then "Sprite.draggable" becomes the token, and will almost certainly never cause a collision.

If we did this, it would mean changing all Leopard properties to use this sort of syntax. That means changes like this:

-this.x += 20;
+this[Sprite.x] += 20;

It would enable sprites to name their own variables this.x and such...

...but may or may not be a pain for actually writing code ✨

Obviously, if we used a syntax like this, apart from being a new major version, we'd need a button in the Leopard editor to automatically update existing code to use the new syntax.

It's worth putting into perspective that draggable is the only property that is blatantly missing. We already have translations for all other sprite-attribute blocks.

Though, we don't have support for more extensions than just Pen... and with that in mind, maybe a syntax based on the category of blocks would be appropriate...?

-this.x += 20;
+this[Motion.x] += 20;

The reasoning here is that users have the opportunity to code their own extensions, the same way we code Motion and Looks and so-on, share those with other Leopard community members, and use them with very much the same syntax as Leopard blocks use. And, of course, there are still no name collisions when we add new blocks / categories of blocks.

What do you think about that? For now, I don't think there's a trouble with sticking with draggable and just bumping the major version. But it might be a useful syntax to use in the future, if it doesn't feel too awkward in practice.

(The only current inconvenience is that, um, I believe all sprites uniquely import right from the https://unpkg.com/... URL. So all of those URLs would be need to be updated at once, to avoid importing two unique versions of Leopard in the same project. Again, this is ideally something the Leopard editor would take care of - assuming we aren't using and/or polyfilling import maps to take care of it more JS-natively.)

PullJosh commented 4 months ago

I think the more verbose syntax would be way too annoying to use in practice (this.[Motion.x] is a great example), so just adding this.draggable and bumping the major version seems wise. But it would be fun to explore possibilities for enabling custom extensions of Leopard. (People have been asking for Turbowarp block support.) I think this deserves a whole other discussion.

It would be neat if we could build a code-mod to help users upgrade Leopard versions automatically. Ideally it would be usable from the CLI but also be integrated directly into the Leopard editor.

In the meantime, newly-generated Leopard projects would use the new major version and that alone would be pretty good.

towerofnix commented 4 months ago

It probably won't happen that Leopard ever adds a new property that all sprites have again (setting aside compatibility with a hypothetical Scratch 4.0), so I don't know that spending the time to develop an entire dedicated tool is worth it. It would functionally just 1) check if you use this.draggable anywhere and ask you rename it, and 2) update unpkg URLs pointing to ^1 to ^2. Both of these are trivial operations in an offline text editor. Less so in the Leopard editor, which is why if we do want an upgrade utility, we feel it should focus on being in the Leopard editor first and foremost.

We disagree that it's annoying in practice, but we're way more tolerant of this "nonsense" than average LOL - that is why we asked. Do note it's this[Motion.x], not this.[Motion.x] (<- 1000x more annoying imo).