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 for pitching the camera #79

Merged
merged 9 commits into from Jan 10, 2017
Merged

Conversation

Herbstein
Copy link
Collaborator

This was a fairly simple implementation. Right now it uses the rotation modifier keys and speed, but I can add separate settings if that is needed.

Do note that I, unfortunately, ran rustfmt on src/core/ui.rs. I can revert and re-implement, if you'd prefer that.

@aeplay
Copy link
Member

aeplay commented Jan 9, 2017

Already getting the cinematic stuff going, I see. Can't you just revert all the unnecessarily changed lines that don't have anything to do?

Also would be nice if you could either not add any default keybindings at all or make them pretty obscure, so you don't accidentally tilt the camera.

@Herbstein
Copy link
Collaborator Author

Just to be clear, this is for looking up and down with the camera, not tilting it sideways. Tilt might have been a bad descriptor. I decided to implement this mostly because I felt that the default angle was too big.

I can definitely remove any usage from the game code itself, but I would personally find it an incredibly useful feature to have.

@aeplay
Copy link
Member

aeplay commented Jan 9, 2017

Oh I see, confusing indeed. Maybe "pitch" like for planes?

...and yes, that is much more useful.

@Herbstein
Copy link
Collaborator Author

That is a far better name for it. I'll change the naming scheme in the source right away.

We also currently have yaw implemented, but it's simply called 'rotation'. Should I rename that too?

@aeplay
Copy link
Member

aeplay commented Jan 9, 2017

Yeah that would be cool and consistent. Thank you!

@Herbstein Herbstein changed the title Support for tiliting the camera Support for pitching the camera Jan 9, 2017
@Herbstein
Copy link
Collaborator Author

This wasn't the place for it, but I do think we should run rustfmt over the source soon-ish. Most editors should be configurable to run the command every time a file is saved; I personally know that Visual Studio Code + Rusty Code supports this natively. Making sure to adhere to rustfmt would result in way cleaner, and more consistent, code.

@Herbstein
Copy link
Collaborator Author

Herbstein commented Jan 10, 2017

Because of the way camera translation works, panning to almost-horizontal makes the camera move very slowly. I don't think there's much we can do right now, but we should be aware of the issue.

Maybe movement speed should be based on the cameras distance to the target instead of the cameras height, since the reason for the original feature was different speeds at different zoom-levels.

@Herbstein
Copy link
Collaborator Author

It seems to work fairly well, but I'm not sure if the camera is a tad bit too slow. At least that is easily changeable.

@kingoflolz
Copy link
Collaborator

Maybe a separate modifier key can be added, and bound by default to the same as the rotational modifier keys, for people who want them separated.

@Herbstein
Copy link
Collaborator Author

Herbstein commented Jan 10, 2017

That is easily added. The only problem with that is that we check for mouse input in an if ... else if ... else expression. Only one of the modifiers can used at a time. Do we rely on that fact, or is that just baggage?

After testing: Removing the else clauses definitely wasn't the solution. I think we need a slightly more robust way of handling movement modifiers.

@kingoflolz
Copy link
Collaborator

kingoflolz commented Jan 10, 2017

I think the problem is that this clause https://github.com/citybound/citybound/blob/master/src/core/ui.rs#L335 needs to be triggered if and only if none of the other branches execute.
Maybe set a flag in each if branch?

@Herbstein
Copy link
Collaborator Author

I decided to do it slightly differently, but the result is ultimately the same. Any other comments/ideas?

@kingoflolz
Copy link
Collaborator

Looks great to me!

@aeplay
Copy link
Member

aeplay commented Jan 10, 2017

Just tried it, works great, thank you!

@aeplay aeplay merged commit 8109336 into citybound:master Jan 10, 2017
@Herbstein Herbstein deleted the tilt_camera branch April 28, 2017 12:34
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.

None yet

3 participants