lambdaisland/cljbox2d

Zoom never sets the zoom, just adds to it?

Closed this issue · 9 comments

Folcon commented

I was trying to use zoom! to change the zoom level to 1px = 1m away from the default of 100px = 1m.

However it was really unclear that zoom! is actually adding to the current zoom level instead of setting it.

In this case a doc to that effect or naming it zoom+! may be better?

So the correct operation is actually:

(b/zoom! -99)

not

(b/zoom! 1)

Otherwise this looks like quite a nice library, thanks for creating it!

plexus commented

Hi @Folcon , I'm happy to see you've found this library. You're the first person (besides me) that I know of that has tried running this, so you are likely to run into some rough edges, but we'll do our best to get them resolved.

I think you're right in this case, it would make more sense to have zoom! directly call through and set an absolute zoom level. Adding a helper function zoom-by! or zoom+! that calls zoom! would be useful too. I guess for what I was doing a sort of relative zoom made more sense, but I agree that it's not intuitive, and it's better to have the more primitive call. Do we have a call for getting the zoom level?

Let me know if you want to take a stab at making a PR.

Folcon commented

@plexus, just so I understand how your naming works, what is the * in zoom*! signifying? I thought it was scaling, ie multiplication, but it's addition?

So we're looking for:
zoom! -> sets absolute zoom level
zoom-by! or zoom+! -> Adds to zoom level

Anything else? Just trying to figure out what consistent names would be =)...

plexus commented

I don't remember exactly but I think in this case it might just mean it's a protocol function, which has a corresponding wrapper without the * which is a regular function, see also alter-user-data*!

I would start by adding a zoom method to IProperty, and implement that for camera. We don't have any way to read the zoom level right now I think. Then zoom! can become absolute zoom, and you can implement your own logic for setting a new zoom level based on the existing zoom level.

zoom+! or zoom-by! can then be added as a convenience, it would simply be

(defn zoom-by! [camera amount]
  (zoom! camera (+ (zoom camera) amount))
Folcon commented

Ok, thinking a little about implementation.

zoom is a scalar, but it's also a transform, (Mat22. (Vec2. zoom-level 0) (Vec2. 0 zoom-level)) by the looks of it, I'm thinking:

  1. Have zoom return the Mat22 transform instead of the zoom-level scalar.
  2. If you feed zoom! a transform or a scalar it correctly sets the absolute zoom value.
  3. zoom-by! can work as you suggested.

I'd vaguely like the set and get to be symmetric with the set accepting both a scalar or a Mat22 as a convenience.

I've also noticed there is an existing set-scale! function, which seems to actually be a zoom!, but it's not super clear that it is the zoom function. From that perspective, is it worthwhile doing this or should we extend the set-scale! function to also handle Mat22's?

Assuming you agree it shouldn't take too long to turn around this =)...

plexus commented

That all sounds good. set-scale! is part of the camera namespace. I think it's ok to have that one there, and still have a zoom! in the top level. It's a little redundant but it'll help people find what they're looking for. In that case maybe make set-scale! polymorphic (can take scalar or Mat22), and then zoom! can just be an alias.

Folcon commented

Cool, let me know what you think. Happy to change stuff if that's not what you were expecting =)...

Folcon commented

@plexus Well this is embarrassing, sorry about this, but it looks like I forgot to delete this before merging.

plexus commented
  • 👍1
Folcon commented

As you've not cut a release yet, I figured I could turn around a new PR =)...