search

sunjay / turtle

Create Animated Drawings in Rust
http://turtle.rs
Mozilla Public License 2.0
562 stars 53 forks source link

Audit Speeds #14

Closed sunjay closed 6 years ago

sunjay commented 7 years ago

Look at the movement and rotation speeds and figure out what values they should be. Compare the different examples to see the effect of the change.

Changing this is a breaking change so we should really try to get it right.

Should we even be using an enum? Or should we allow for arbitrary values? We would still need to limit the speed and set a scale factor (i.e. what the speed actually means), so maybe not having an enum isn't worth it? Maybe we should just have 0-100 speeds and 100 should be some very large number? Maybe 20 speeds is enough? We could start with our current 10 and then increase it to 20 later?

Piripant commented 6 years ago

Is there any specific reason why would we set a limit to the speed? I think a simple and straightforward way of representing the speed would something like this

enum Speed {
    Value(f64),
    Instant,
}

Where Value represents the speed in units per second, and Instant represents an instantaneous movement. This way the users can choose freely which speed to choose, without being limited.

sunjay commented 6 years ago

Is there any specific reason why would we set a limit to the speed?

This is the question I plan to explore through this issue. The reason we have 10 speeds is because the Python implementation's speed method only allows 10 speeds. This library was created based on that implementation.

That being said, I personally find having only 10 speeds pretty limiting. There are always cases where I feel like I want the turtle to go faster. Note that this desire to go faster isn't necessarily solved by changing the number of speeds. It has more to do with what absolute speed (in pixels per second) that each speed denomination maps to.

So why not have arbitrary speeds with no limits? Well, not every speed is valid. For example, even though it might be okay mathematically, we don't want to allow things like negative speeds. If we did, the predictability of methods like forward and backward would completely go away. There is also a strict upper limit on how fast the turtle can actually move because of limitations of the speed of IPC, rendering, the CPU, etc. After a certain point, every speed just looks like instant. Keeping the speed limited allows us to finely control it, tweak it, and expand the options later if we need to without introducing a breaking change.

I think a simple and straightforward way of representing the speed would something like this

The solution you suggested might work, but it is functionally equivalent to an f64. Instant just means a speed of infinity (f64::INFINITY) in turtle, so we could represent your entire speed type with just f64.

Here's a solution I would be okay with: We have 30 or 50 speeds. Speed is just an integer, no enum. The speed method would change to return an integer. Nothing else changes other than that and that there is no more speed enum.

Our public API already supports passing in both integers and floating point numbers for speeds, so this would be a natural transition for anyone writing idiomatic turtle code.

Passing in a value greater than the limit or less than 0 just results in instant (same as current behavior). The default speed is still something around 5 or 6. That way you can still go slower if you want (rarely needed), but also go way faster (often needed). If we need to in the future, we can extend the values to 100 to add more speeds, but I'll adjust the values so that probably isn't needed. For that expansion to not be a breaking change we would probably need to panic for values greater than the limit instead of accepting them and setting the speed to instant.

Note: The reason speed needs to be considered so much now is because even adjusting how fast the values are is a breaking change.

I'm open to feedback and other suggestions too. Hope that clears up some of the design decisions for speed. :)

Piripant commented 6 years ago

Thanks, that makes it all clear. I never used the python Turtle API so I didn't know, sorry. I think using only an integer instead of a float/enumerator would be a good idea, and even an unsigned integer (u8) could be used to disallow negativity completely. I think a nice way would be for the value to express the speed as a percentage (1 to 100), where 1 is the slowest and 100 is full maximum speed. I think 0 could be used to express instant speeds. This may deviate from the python API, but still is intuitive enough to not be a bordering difference in my opinion.

sunjay commented 6 years ago

Thanks, that makes it all clear. I never used the python Turtle API so I didn't know, sorry.

No need to apologize. :) I'm happy to answer any and all questions about turtle and its design.

I think using only an integer instead of a float/enumerator would be a good idea, and even an unsigned integer (u8) could be used to disallow negativity completely.

I'm going to need to think about this more because we want to use a type that can be returned from .speed() and then used in other calculations. I'm not yet sure if the best choice is i32, u32, u8, or something else. Since it's just one number, I don't really care if it wastes space if that means better usability overall. A u8 is nice when you know you're only going to store values up to 255, but it doesn't necessarily interop well with other types. Again, I'm not sure what to pick yet, so I'll have to think about it more before I come up with a final type.

I think a nice way would be for the value to express the speed as a percentage (1 to 100), where 1 is the slowest and 100 is full maximum speed. I think 0 could be used to express instant speeds. This may deviate from the python API, but still is intuitive enough to not be a bordering difference in my opinion.

This is certainly an option! I think I'd like to choose something else though because this solution has the same kinds of problems as the 1 to 10 system we have now. If we say that 100% is the maximum speed, we can't add more numbers past it in the future if we need to. I like what I suggested because we can expand the options later to fit future needs.

I think 0 could be used to express instant speeds.

I was thinking about this too. I kind of don't like this because it messes with the mathematical meaning of speed. If someone writes turtle.speed() * 200, they will be surprised at the results when speed is 0. Maybe speed() should return a value between 1 and the limit? I think the enum you proposed earlier would be a good choice to represent speeds if we go with an integer type.

enum Speed {
    Value(u8),
    Instant,
}

Another Approach

In turtle, speed doesn't just represent one quantity. It maps to both movement speed and rotational speed. That is one of the reasons I kept it as a field-less enum. In that form, its value can't be destructured (pattern matched) and used in calculations.

Perhaps we shouldn't expose anything about speed at all. We could wrap speed in a type using the newtype pattern and limit its operations. We could implement Ord, Eq, Add, Sub, etc. and allow speed to be compared and manipulated with integers, but then always return a new speed. We could disallow getting the inner value of a speed and make the to_absolute and to_rotation methods public.

https://github.com/sunjay/turtle/blob/4a9d83db19d2b00814a910c0602ec4a9c153bb95/src/speed.rs#L27-L65

That way, people could still do things like turtle.set_speed(turtle.speed() * 2), but they couldn't treat the integer value of speed as if it actually represents something (like movement or rotational speed).

I'm a big fan of this approach as it distinguishes speed from just a regular number (since speed in turtle represents two numbers) and it hides all of the implementation details so we can continue to change it in the future.

Thanks for having this discussion with me! This is bringing out a lot of the different alternatives! I don't think I'd be thinking this deeply about this without your comments and questions. :smile:

sunjay commented 6 years ago

To anyone reading this thread: Feel free to let me know if I'm talking about concepts in Rust that you don't understand or know about. I or someone else will point you to some resources to learn more. There's nothing wrong with not knowing and your contributions to the discussion will get better as you understand more of what is going on. :)