-
Notifications
You must be signed in to change notification settings - Fork 87
Android build #1804
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
base: master
Are you sure you want to change the base?
Android build #1804
Conversation
|
Changes:
|
|
Git History should be cleaned up (Some things should be squashed, so that some distinguish commits are there) |
|
|
||
| add_subdirectory(WinAPI) | ||
| add_subdirectory(SDL2) | ||
| add_subdirectory(SDL2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change
| ) | ||
| add_dependencies(drivers videoSDL2) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change
libs/s25main/WindowManager.cpp
Outdated
| if(time_now - lastLeftClickTime < (VIDEODRIVER.IsTouch() ? DOUBLE_CLICK_INTERVAL / 3 : DOUBLE_CLICK_INTERVAL)) | ||
| { | ||
| mc.dbl_click = true; | ||
| } else | ||
| if(mc.GetPos() == lastLeftClickPos) | ||
| mc.dbl_click = true; | ||
| else if(VIDEODRIVER.IsTouch()) // Fast unmöglich 2 mal auf den exakt selben punkt zu tippen | ||
| { | ||
| // Wenn doppeltippen -> fenster schließen | ||
| IngameWindow* window = FindWindowAtPos(mc.GetPos()); | ||
| if(window && !window->IsPinned()) | ||
| window->Close(); | ||
| } | ||
| } | ||
|
|
||
| if(!mc.dbl_click) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that logic is a bit weird. But I don't have a good idea here.
@Flamefire do you have an idea to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous logic not so much. The new one mixes double-click detection with window closing.
This should only be for double-click detection. For Touch you could have a "points-close" check. And in the window you could handle a double-click while touch as close command.
But the current solution stays weird. IIRC we have windows where you select an entry with double-click. Now this will close the window instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous logic not so much. The new one mixes double-click detection with window closing.
This should only be for double-click detection. For Touch you could have a "points-close" check. And in the window you could handle a double-click while touch as close command.
But the current solution stays weird. IIRC we have windows where you select an entry with double-click. Now this will close the window instead.
So what could I do?
Should the double click be handled in the window class and only close when IsTouch & DblClick is not needed to interact with that part of the window?
Which windows do even need a dbl click to work? Never seen one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which windows do even need a dbl click to work? Never seen one.
ctrlList, ctrlTable use it for selection.
Double-click on window titlebars maximizes/minimizes the window
So yes handling it in IngameWindow when not otherwise handled is the only sensible option.
IIRC the IngameWindow::MouseLeftUp won't be called if a control handled it. If that is the case you can add it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which windows do even need a dbl click to work? Never seen one.
ctrlList, ctrlTable use it for selection. Double-click on window titlebars maximizes/minimizes the window
maximize/minimize won't work for me.
I think the wrong config is being used... windowpinning for minimizing??
in IngameWindow.cpp
switch(btn)
{
case IwButton::Close: Close(); break;
case IwButton::Title:
if(SETTINGS.interface.enableWindowPinning && mc.dbl_click)
{
SetMinimized(!IsMinimized());
LOADER.GetSoundN("sound", 113)->Play(255, false);
}
break;
But I could swear it worked some time ago. Is this wanted or should I correct it?
Could just do this:
switch(btn)
{
case IwButton::Close: Close(); break;
case IwButton::Title:
if(!mc.dbl_click)
break;
case IwButton::PinOrMinimize:
if(SETTINGS.interface.enableWindowPinning)
{
SetPinned(!IsPinned());
LOADER.GetSoundN("sound", 111)->Play(255, false);
} else
{
SetMinimized(!IsMinimized());
LOADER.GetSoundN("sound", 113)->Play(255, false);
}
break;
}
IIRC the IngameWindow::MouseLeftUp won't be called if a control handled it. If that is the case you can add it there.
I don't think this is necessary. Every window which makes sense to quick-close works with its mouseLeftUp function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maximize/minimize won't work for me.
I think the wrong config is being used... windowpinning for minimizing??
It is working as intended, see description of 5b33ede
Add a "pin" function to in-game windows. The function replaces the minimize button and the window can instead be minimized by double-clicking the title bar.
Not fully obvious but reasons seems to be that the button is repurposed for the pinning.
IIRC the IngameWindow::MouseLeftUp won't be called if a control handled it. If that is the case you can add it there.
I don't think this is necessary. Every window which makes sense to quick-close works with its mouseLeftUp function
My point was: You can add the close-functionality to IngameWindow::MouseLeftUp if and only if it doesn't get called if a control already handled it. Otherwise the window might be closed when it shouldn't.
Hence you'd need to double check that it really works this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the IngameWindow::MouseLeftUp won't be called if a control handled it. If that is the case you can add it there.
I don't think this is necessary. Every window which makes sense to quick-close works with its mouseLeftUp function
My point was: You can add the close-functionality to IngameWindow::MouseLeftUp if and only if it doesn't get called if a control already handled it. Otherwise the window might be closed when it shouldn't. Hence you'd need to double check that it really works this way
I think I've already done this in my commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. But I wasn't just referring to the title-bar icons but to any "control", i.e. button, list, etc.
In particular in a table control as used in e.g. the save/load windows a double-click means selection.
Nos I fear 2 things can happen:
- The user missed the intended row and the window just closes
- The action could not be performed and the window should stay open but gets closed
However I can't find a way to properly handle that with out current UI system. We have means to detect a "handled" event we don't use it. Not only that an ingamewindow has Msg_LeftUpand MouseLeftUp. The latter doesn't seem to make sense to me as we have the former for that.
I think this works:
if(!activeWnd.RelayMouseMessage(&Window::Msg_LeftUp, mc))
activeWnd.Msg_LeftUp(mc);
Similar for the other events but that needs some more careful thought.
I'm reasonably sure that for now changing WindowManager.cpp like this would suffice for most cases:
// ja, dann Msg_LeftUp aufrufen
IngameWindow& activeWnd = *windows.back();
- activeWnd.Msg_LeftUp(mc);
-
- // und den anderen Fenstern auch Bescheid geben
- activeWnd.RelayMouseMessage(&Window::Msg_LeftUp, mc);
-
- // und noch MouseLeftUp vom Fenster aufrufen
- activeWnd.MouseLeftUp(mc);
+ if(!activeWnd.RelayMouseMessage(&Window::Msg_LeftUp, mc))
+ {
+ activeWnd.Msg_LeftUp(mc);
+ activeWnd.MouseLeftUp(mc);
+ }This should at least avoid triggering your close code when the event was already handled. From some searching I'm reasonably sure that this doesn't cause any issues: MouseLeftUp only handles the titlebar and so no control could have handled that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code breaks not just the touch-dblclick-close but also dblclick with mouse on the title bar which locks left click. (Window gets moved around with cursor)
I've issues understanding this part of the code and I'm not sure how to solve this problem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check in the next couple of days, that was just thinking what could work, haven't tested it.
libs/s25main/Settings.h
Outdated
| { | ||
| unsigned autosave_interval; | ||
| bool invertMouse; | ||
| unsigned mouseMode; // default/original:0 inverted:1 natural:2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an enum then. Also what do those 3 setting mean?
Map moves with your cursor when scrolling/panning
This sounds like just "inverted" (to original)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an enum then. Also what do those 3 setting mean?
Map moves with your cursor when scrolling/panning
This sounds like just "inverted" (to original)
inverted is the same as the previous 'invert_mouse' setting (which I removed/replaced)
Natural is 1to1 moving the map with mouse like in most games
In rttr the map movement never was 'grab map and drag around'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't inverted that grab map and move? Or does it mean it doesn't endless scroll as the cursor actually moves inside the window together with the map instead of staying where it is?
Maybe add a short description to the enum
Then maybe rename it to "mouseMapMoveMode" or something better. And if you can come up with enum names that convey that very succinct then it's perfect!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't inverted that grab map and move? Or does it mean it doesn't endless scroll as the cursor actually moves inside the window together with the map instead of staying where it is? Maybe add a short description to the enum
Smart cursor: off
original:
When moving mouse to left, the map moves to the right
inverted:
When moving mouse to left, the map moves to the left
natural:
When moving mouse to left, the map moves to the map moves to the left
Smart cursor on:
original, inverted:
Same as above but mouse stays on the same position & I think the acceleration is higher
natural:
Same as above
Every mode moves the map faster than the cursor but natural lets the user drag the map
Then maybe rename it to "mouseMapMoveMode" or something better. And if you can come up with enum names that convey that very succinct then it's perfect!
Wouldn't "mouseMapMoveMode" be pretty long? this would result in mouse_map_move_mode in the config...
If you don't like mouseMode what do you think of mapScrollMode or something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would result in mouse_map_move_mode in the config...
I don't have an issue with that at all. It has to be descriptive
mapScrollMode sounds ok. I have more of an issue with "natural" as that is highly ambiguous. The "normal" mode is "natural" to many (old) gamers.
Maybe something like "sticky", "stickToCursor" or so? Yeah, naming is hard :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an enum then. Also what do those 3 setting mean?
An local enum or an global enum class? I'm not sure what would be better. And where would I define that enum class?
I could use an local enum in Settings.cpp and dskGameInterface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Settings.h seems best as a global enum(class). Everyone using only does through this class/header anyway.
It can be global as Settings::MapScrollMode::Original is unnecessarily long and the enum class makes it "scoped" already
Maybe something like "sticky", "stickToCursor" or so? Yeah, naming is hard :-/
A better idea: "Grab" or "GrabAndDrag" as that seems to describe it best: You don't scroll but you drag the map with your mouse/finger. Then maybe MapMoveMode for the enum and possibly ScrollSame & ScrollOpposite for map movement in the same direction as the mouse and the other way to be clearer than "original/inverted"
A bit confusing: in Original mode the map moves in the opposite direction as the mouse so the view moves in the same direction as the mouse. While the "revert/invert" mode is the opposite.
As with "GrabAndDrag" we refer to dragging the map I guess ScrollOpposite would be the current "original"
@Flow86 Your thoughts?
| if(!path.empty()) | ||
| { | ||
| LOG.write("Note: %1% path manually set to %2%\n", LogTarget::Stdout) % id % path; | ||
| } else | ||
| return defaultPath; | ||
|
|
||
| return path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd try to avoid negated checks. Especially here it might be natural the other way round
| if(!path.empty()) | |
| { | |
| LOG.write("Note: %1% path manually set to %2%\n", LogTarget::Stdout) % id % path; | |
| } else | |
| return defaultPath; | |
| return path; | |
| if(path.empty()) | |
| return defaultPath; | |
| LOG.write("Note: %1% path manually set to %2%\n", LogTarget::Stdout) % id % path; | |
| return path; |
libs/s25main/Settings.h
Outdated
| { | ||
| unsigned autosave_interval; | ||
| bool invertMouse; | ||
| unsigned mouseMode; // default/original:0 inverted:1 natural:2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would result in mouse_map_move_mode in the config...
I don't have an issue with that at all. It has to be descriptive
mapScrollMode sounds ok. I have more of an issue with "natural" as that is highly ambiguous. The "normal" mode is "natural" to many (old) gamers.
Maybe something like "sticky", "stickToCursor" or so? Yeah, naming is hard :-/
libs/s25main/WindowManager.cpp
Outdated
| } else if(time_now - lastLeftClickTime < TOUCH_DOUBLE_CLICK_INTERVAL) | ||
| { | ||
| // Calculate distance between two points | ||
| unsigned cDistance = SQR(mc.pos.x - lastLeftClickPos.x) + SQR(mc.pos.y - lastLeftClickPos.y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a hint for future changes: Use const auto diff = mc.pos - lastLeftClickPos to avoid the potential for C&P issues due 2 repeated use of x and y in the same line. The below could then be if(square(diff.x) + square(diff.y) <= square(TOUCH_MAX_DOUBLE_CLICK_DISTANCE)) which explains why the RHS is squared more directly.
No need to change this, just a suggestion for future changes as this is mostly an opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Calculate distance between two points
const auto diff = mc.pos - lastLeftClickPos;
if(square(diff.x) + square(diff.y) <= square(TOUCH_MAX_DOUBLE_CLICK_DISTANCE))
mc.dbl_click = true;
diff would be Point<int> but TOUCH_MAX_DOUBLE_CLICK_DISTANCE is unsigned which can't be compared.
Do you know a better way than changing the constant to int or using static_cast in the if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only idea: Make square return std::make_unsigned_t<T> and do the cast there as the square of an int will always be positive.
| return true; | ||
| } | ||
|
|
||
| void dskGameInterface::Msg_ButtonClick(const unsigned ctrl_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you move those methods around? Any reason for that? The diff is very large and I can't tell if there were changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not.
I've just created the ContextClick function above. Don't know why the patch was created this weird.
| /// Updatet das Post-Icon mit der Nachrichtenanzahl und der Taube | ||
| void UpdatePostIcon(unsigned postmessages_count, bool showPigeon); | ||
|
|
||
| /// Wird beim Linksklick ausgeführt und überprüft die klick Position auf Gebäude/Straßen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English please
| CHECK_SDL(SDL_GetDisplayUsableBounds(SDL_GetWindowDisplayIndex(window), &usableBounds)); | ||
| int top, left, bottom, right; | ||
| CHECK_SDL(SDL_GetWindowBordersSize(window, &top, &left, &bottom, &right)); | ||
| if(SDL_GetWindowBordersSize(window, &top, &left, &bottom, &right) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Farmer-Markus Can you do the latter so that we don't loose the error reporting?
libs/s25main/Settings.cpp
Outdated
| } catch(const std::runtime_error&) | ||
| { | ||
| // ScrollSame(old invertMouse) is 0 so invert value | ||
| interface.mapScrollMode = static_cast<MapScrollMode>(!iniInterface->getBoolValue("invert_mouse")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That cast can't be verified at this point. Just do a ternary here to set the enum value directly. So you don't even need the comment
This will also throw an error if the value does not exist, so please keep the prior one with default value
I don't think you need the warning either as there is nothing to check: The old setting is translated 1:1 to the new one
libs/s25main/WindowManager.cpp
Outdated
| if(time_now - lastLeftClickTime < (VIDEODRIVER.IsTouch() ? DOUBLE_CLICK_INTERVAL / 3 : DOUBLE_CLICK_INTERVAL)) | ||
| { | ||
| mc.dbl_click = true; | ||
| } else | ||
| if(mc.GetPos() == lastLeftClickPos) | ||
| mc.dbl_click = true; | ||
| else if(VIDEODRIVER.IsTouch()) // Fast unmöglich 2 mal auf den exakt selben punkt zu tippen | ||
| { | ||
| // Wenn doppeltippen -> fenster schließen | ||
| IngameWindow* window = FindWindowAtPos(mc.GetPos()); | ||
| if(window && !window->IsPinned()) | ||
| window->Close(); | ||
| } | ||
| } | ||
|
|
||
| if(!mc.dbl_click) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check in the next couple of days, that was just thinking what could work, haven't tested it.
9978c52 to
58e0d92
Compare
58e0d92 to
1556dcc
Compare
I'm working on recreating my android port of rttr.
Currently the game starts but failes to find any drivers.I would need an option to set a custom output path for the translation/data copying.Also an enviroment variable to set the path to the drivers or the game data (or both)
Also another problem is openGl.
On android only openGl-Es is available and I've used a translation library (gl4es) that works pretty well. But the SDL videodriver would need: 1. to link against gl4es and 2. the SDL_gl_getprocaddress to be replaced by the gl4es equivalent.
This could also be used to support devices like the raspberry pi.
Also I would implement basic touch controls which should also work on touchscreen laptops (thanks to sdl)
I know these would be some pretty big changes...