New camera modes #1454

Merged
merged 45 commits into from Sep 23, 2012

Projects

None yet

6 participants

@GizmoR13
Contributor

Based on @Luomu work #1125.

New camera modes,
cameras are placed around the ship.

Numeric keypad:

7 - Front cockpit view
1 - Rear cockpit view
8 - Front camera
2 - Rear camera
9 - Top camera
3 - Bottom camera
4 - Left camera
6 - Right camera
DIVIDE - External view
MULTIPLY - Sidereal view
(+) - Zoom in
(-) - Zoom out

TAB - on/off display camera name

Sequence when click on the icon:

Front cockpit view
Rear cockpit view
Front camera
Rear camera

with left CTRL:

Left camera
Top camera
Right camera
Bottom camera

with left CTRL and left SHIFT:

External view
Sidereal view

Closes #1130, partially solves #1105.

Owner
Luomu commented Aug 27, 2012

We are drowning in buttons! And also in view modes.

Instead of treating up/down/etc as separate views, this should be all organized as part of the internal camera mode.
Same keybinds should be used as much as possible across different views, so "Look Left" rotates the camera in external view.

All camera controls can be condensed to (note - all controls need to be rebindable so the precise keys can are just suggestions):

  • Shift + X - Switch camera mode
  • Numpad 5 - Cycle forward/back
  • Numpad 4 - Look left
  • Numpad 6 - Look right
  • Numpad 8 - Look up
  • Numpad 2 - Look down
  • Numpad 1 - Roll left
  • Numpad 3 - Roll right
  • (+) - Zoom in
  • (-) - Zoom out

What the buttons precisely do depends on the mode. Internal view will not do a roll.

After this, the view modes are:

  • Cockpit view
  • External view
  • Sidereal view

Rear camera is to be removed since it is part of the cockpit view.

Showing the view name on screen is pretty much how I wanted it to be, as a tweak I think it would be good to fade it out after a couple of seconds. The name should be shown each time a view is exposed (on camera switch or returning from starmap etc.)

View mode button on the console can be removed at some point. It is too clumsy for switching many modes.

Contributor

Peter, a few days ago that I'm testing your work. I feel very good that can handle all functions with the numeric keypad. I think you have studied well the ergonomic aspect. But, I think he's used to F1 etc, feel uncomfortable. I think you should work on that aspect. Regarding the rear cockpit camera, I think someone will think to give utility.

Contributor

Thanks for comments.
I do not have too much time at the moment, but soon i will try change the view as @Luomu suggested.

Contributor

I hope @Luomu reconsider begin using the numeric keypad. :)

Owner
robn commented Sep 1, 2012

Numeric keypad is a nice idea, but lets not forget that many many computers (most laptops) don't have keypads. Of course, keys will be rebindable at some point so its not a huge issue as long as we set up reasonable defaults.

There certainly is a lot of camera objects now! I wonder if just repointing/repositioning the camera on each switch would make more sense? (@Luomu, was that what you meant by "they should be organised as part of the internal camera mode"?)

Contributor
walterar commented Sep 1, 2012

Something that should be considered is the difference in keyboards. The Spanish, for example, runs "-" (zoom) but no "+", and other. Apparently the only support all languages ​​is the numeric keypad. :(

Owner
Luomu commented Sep 1, 2012

I mean left, right, up, down, front and rear views should be viewpoints of the internal camera mode, not camera modes themselves.

Anyway, my core message was:

  • avoid viewmode-exclusive keybinds as much as possible
  • everything should be rebindable (so it doesn't matter if $look_left bound to numpad 4 is not perfect for everyone)
Contributor
GizmoR13 commented Sep 5, 2012

Ok, @Luomu, how does it look now?

Owner
Luomu commented Sep 5, 2012

Looks a lot better from from the diff! I'll actually try it later today.
It looks like there are still too many keybinds: there should be no KeyAction leftCamera, only cameraRotateLeft.
I'm very picky about this because camera is one of those things that needs to be perfect. In any case, I don't think it's a problem even if we take it in the current state.

@Luomu Luomu and 1 other commented on an outdated diff Sep 5, 2012
src/WorldView.cpp
}
+
+ int i;
+ for (i=0; cameraName[i]; i++)
Luomu
Luomu Sep 5, 2012 Owner

Something odd is going on here

johnbartholomew
johnbartholomew Sep 5, 2012 Contributor

It's actually correct, except I'd move the declaration of int i into the for loop header because this is C++, not C89. It's looping over characters in the cameraName string. The test is checking for the null terminator.

johnbartholomew
johnbartholomew Sep 5, 2012 Contributor

Actually... technically I don't think you're meant to rely on std::string having a null terminator, or if it does have one I don't think you're supposed to rely on being able to access it safely through operator[]... so yeah, this loop should be changed.

johnbartholomew
johnbartholomew Sep 5, 2012 Contributor

If you want the really C++ way, it would be:

std::transform(cameraName.begin(), cameraName.end(), cameraName.begin(), (int (*)(int))std::toupper);
Owner
Luomu commented Sep 5, 2012

Crashes on startup (tried earth and EE start points)

    pioneer-debug.exe!std::basic_string<char,std::char_traits<char>,std::allocator<char> >::operator[](unsigned int _Off)  Line 1441 + 0x17 bytes   C++
    pioneer-debug.exe!WorldView::SetCamType(WorldView::CamType c)  Line 310 + 0x24 bytes    C++
    pioneer-debug.exe!WorldView::InitObject()  Line 212 C++
    pioneer-debug.exe!WorldView::WorldView()  Line 36   C++
    pioneer-debug.exe!Game::CreateViews()  Line 611 + 0x2e bytes    C++
    pioneer-debug.exe!Game::Game(const SystemPath & path)  Line 47  C++
Contributor
GizmoR13 commented Sep 8, 2012

It should be OK now.

Contributor

I don't see anything else.

Contributor

Rebased with master.

Contributor

to keep up to date.

Contributor

This seems good, but I'm concerned about using the numpad by default, because as @robn pointed out, laptop users usually don't have a numpad. It would be great if we could detect the presence of a numpad and pick a default based on that, but I don't think it's possible.

We currently use the arrow keys to rotate the view in external and sidereal view. Maybe we could set the defaults to arrow keys for Front/Rear/Left/Right, Shift+Up for Top, Shift+Down for Bottom, Ctrl+Up for cockpit front, Ctrl+Down for cockpit rear? That should also mean the current keys still work so it's not confusing for people who aren't expecting the change.

Contributor

Cross-ref: This StackOverflow question Detecting the presence/absence of a numeric keypad?

Contributor

I think it should stay as it is.

I think that the use of the numeric keypad is the most ergonomic.
We should offer users the best ergonomics,
and if for some reason they can not use them,
They can easily change it.

In addition, in the future,
can be added default keyboard settings
for different types of keyboards
but I'm talking here about all the keys, not just the view.

@Luomu Luomu commented on the diff Sep 23, 2012
src/KeyBindings.cpp
@@ -29,6 +29,24 @@
KeyAction decreaseScanRange;
KeyAction toggleHudMode;
+KeyAction frontCockpit;
+KeyAction rearCockpit;
+KeyAction frontCamera;
+KeyAction rearCamera;
+KeyAction leftCamera;
+KeyAction rightCmaera;
Owner
Luomu commented Sep 23, 2012

You still have separate keybinds "leftCamera" and "cameraRotateLeft", it should be the one and the same action. Anyway, if this works, just merge it, I don't want to stall it.

I don't think we are required to make a decision if numpad is good or not, players can rebind the keys.

@johnbartholomew johnbartholomew merged commit cbb2c14 into pioneerspacesim:master Sep 23, 2012
Contributor

I don't like that we've just broken the game by default for laptop users. However, in the interests of moving this forward I have merged anyway. When the input system gets improved (which it will eventually) I hope we can fix this in a nicer way. In the meantime laptop users will have to adjust their key bindings.

Contributor

ÓĘŁ... -> óęł...
It's bug or only my compiler problem? VC2010

String:

  • EXTERNAL_VIEW
  • Widok zewnętrzny

Contributor

It's bug or only my compiler problem?

It's a bug. The easiest way to fix it is to remove the case conversion code (WorldView.cpp, line 319) and change the text in the language files to upper-case.

The other way to fix it is to use a full Unicode case mapping function, but we would have to use an external library for that because it gets quite complicated.

Contributor

Or we could just remove the case conversion and leave the language files as they are. The text in the view doesn't really have to be upper-case.

Contributor

Looks good after deleting this line.

Contributor

If this does not make the other problem.

@johnbartholomew johnbartholomew pushed a commit to johnbartholomew/pioneer that referenced this pull request Sep 24, 2012
John Bartholomew remove broken transform to uppercase on the WorldView camera name label
This doesn't deal with UTF-8 strings properly. If we want it we'll need
to use a real Unicode case mapping function from some external library.

Noted in #1454 after the PR was merged.
c0fda8e
@johnbartholomew johnbartholomew added a commit that referenced this pull request Sep 27, 2012
@johnbartholomew johnbartholomew save bump (for #1454)
I forgot again.
528b7c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment