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

Save minetest screen width/height options when modified #5683

Merged
merged 3 commits into from May 5, 2017

Conversation

nerzhul
Copy link
Member

@nerzhul nerzhul commented Apr 30, 2017

capture d ecran de 2017-05-04 16-45-22

@bigfoot547
Copy link
Contributor

I think that this could be useful!

@paramat
Copy link
Contributor

paramat commented May 1, 2017

Needs a little more description. Do you mean this saves screen dimensions set by dragging the edges of the screen?
Screen dimensions are already settable in .conf and in advanced settings, and i expect this is how most players set their startup screen size (i have never set screen size by dragging because it causes nasty flicker). They may drag the screen to a different size per gaming session but probably want the default size to remain unchanged.
So seems unnecessary at best, and possibly worse.
Sorry, 👎

me, like every minetest player, should at each minetest launch resize window, it's not a friendly gaming experience

That's an odd assumption, i expect almost all players set their favourite screen size in .conf or advanced settings, surely you do this too?

@pilino1234
Copy link
Contributor

but probably want the default size to remain unchanged.

wat. How do you know this?? I, for one, want to keep the same window size as the last time I used a program, since I resize windows to fit more nicely on my screen. I do not want to fiddle with window sizes before I start playing a game.

i expect almost all players set their favourite screen size in .conf or advanced settings, surely you do this too?

I don't think anybody sets their preferred window size in advanced settings. I don't know the width and height values for my preferred MT window size, I just eyeball it whenever I resize it, how am I supposed to get those values and put them in the probably most badly placed advanced settings fields? (Client -> Graphics -> In-Game (wtf?!) -> Advanced (what's so 'advanced' about screen resolution anyway?))

I have never seen a program have its users set their preferred window size by putting two arbitrary numbers in a settings field. Not user friendly.
If games support a fullscreen mode, they often allow picking from a set of common resolutions, which seems to be the best system, seeing that it's used in so many games.

@nerzhul
Copy link
Member Author

nerzhul commented May 1, 2017

I agree with @pilino1234 editing configuration by hand is ABSOLUTELY NOT user friendly Do you really play games @paramat ? Do you know games which needs to resize the screen at each startup when modified ?

Only thing i can add to make a consensus between users and minetest.conf editors is adding a boolean (default true) to enable auto saving window size (maybe show it in normal settings), this will permit advanced users to disable it, and let regular users use natural window saving way. ok @paramat for that ?

@pilino1234
Copy link
Contributor

I would definitely be okay with an option to disable automatic window size restoring, since some users may not want it. But users should be able to get their preferred window size back on startup automatically if they want to, without having to resize the window manually before they can start playing every single time.

@rubenwardy
Copy link
Member

rubenwardy commented May 1, 2017

I feel like that this will cause more problems than it solves, but it's worth getting user feedback

@rubenwardy rubenwardy added the User feedback needed Feedback from players/server owners/modders is needed to determine if the change should be done. label May 1, 2017
@nerzhul
Copy link
Member Author

nerzhul commented May 1, 2017

I opened a poll on the forum

https://forum.minetest.net/viewtopic.php?f=5&t=17488

@pilino1234
Copy link
Contributor

this will cause more problems

Hmm, such as? Tbh, I can't think of any problems this would cause. It simply enhances user friendliness by implementing a feature found in lots of other games and software already.

@Dumbeldor
Copy link
Contributor

I think that's a good thing to do. Most games do. For the user it is much better.
It's better to wait for the return of players on the forum.

@paramat
Copy link
Contributor

paramat commented May 1, 2017

Do you know games which needs to resize the screen at each startup when modified ?

You don't need to as i wrote above, once you have set your preferred size. if you are resizing your screen every session you are doing something unnecessary, just set the setting.
I don't care what other games do, that is sheep mentality and doing something for no good reason, it's an invalid argument, we should just do what is best.

I will not oppose if it is an option. It's quite likely that many players see resizing by dragging as a per-session thing, to fit the window to other windows currently open, and would want the default dimensions used on next startup. So i don't think the default of the option is so obvious.

I do think screen dimensions are so basic they should be in basic settings menu, as it's one of the first things that needs adjustment for a new player, advanced settings only is not good.

@paramat
Copy link
Contributor

paramat commented May 1, 2017

I don't think anybody sets their preferred window size in advanced settings.

Well i do, so that's 1 at least, and video makers may need to set precise dimensions.
Monitor and TV proportions are chosen because proportions that are based on simple ratios are more pleasing to the eye. So many people want to set precise pixel values.

I have never seen a program have its users set their preferred window size by putting two arbitrary numbers in a settings field

Ubuntu terminal does, it resets to default every time, you have to set columns and rows to set the default.

@bigfoot547
Copy link
Contributor

Speaking of terminals, openbox tells me the dimensions of the terminal window. I think that that could be a useful feature (I will add a screenie when I can get on my comp.). I could make openbox do this, but then it would be on ALL resizable windows.
And maybe a "Reset Width/height" button because with a high-sensitivity mouse, you would need to be a surgeon to reset back to 800x600.

@rubenwardy
Copy link
Member

I don't think that paramat's usage is very typical. I don't think many people manually set width and height, unless they're doing fullscreen.

I think that most people just maximise the window, or snap it to a corner. That's pretty easy to do, and you won't get any problems. With screen saving, you'll get problems if you change monitors or the resolution of your screen decreases.


Nearly all people will just maximise the window, I don't really see the point of this when you can just do that.

Also, shouldn't most window managers handle this for you? Or does Irrlicht override that by manually setting the width and height?

@srifqi
Copy link
Member

srifqi commented May 2, 2017

Can we just add Set this value to current window size button in Advanced Settings dialog? Like Blender app does.

@adrido
Copy link
Contributor

adrido commented May 2, 2017

Finally, a long awaited feature! Thanks! 😄

Would it also possible to save and restore the maximized state?

@Zeno-
Copy link
Contributor

Zeno- commented May 2, 2017

The main problem is if there is a bug with whatever window manager the player is using and the window gets resized to basically w=0,h=0 (this has happened to me with Plasma). Apart from that I think that the idea that a user will go to a smaller screen size is not very likely but a check on startup to make sure that the window size is not larger than the screen size would be fine (maybe the code already does this, I don't know).

Restoring the window size to what was used in a previous session is a fairly common and accepted thing.

@Zeno-
Copy link
Contributor

Zeno- commented May 2, 2017

Why is user feedback required? This is not controversial... it just requires that a decision be made :/

@nerzhul
Copy link
Member Author

nerzhul commented May 2, 2017

Yeah Zeno, you are right, it miss the check that the restored screensize is valid, i will add this

@SmallJoker
Copy link
Member

SmallJoker commented May 2, 2017

The idea itself sounds very good but without (yet another) setting it might be annoying to adjust the size parameters to take uniform screenshots or videos.
Not a too important issue for me, so +1 on the concept.

@paramat
Copy link
Contributor

paramat commented May 2, 2017

Thinking further on how i use other applications, i find sometimes i want a drag-resize stored, sometimes i don't, both behaviours are very important so i feel this being an option is essential, i will not oppose if made an option.
My usage is probably in the minority, i assumed that most would want to set their exact pixel dimensions or proportion to often used values, i guess most others just don't care.
So -1 for current PR, if optional i'm neutral.

@sfan5 sfan5 removed their request for review May 3, 2017 11:54
@paramat
Copy link
Contributor

paramat commented May 3, 2017

Changed my opinion, i'm neutral now. I think players would like the option (the poll suggests an option default true) but i can see how saving a drag-resize is probably the most wanted behaviour and the most useful for new players who need to resize immediately.

@nerzhul
Copy link
Member Author

nerzhul commented May 3, 2017

i'm not very formspec aware but i will try to add the checkbox, and verify window size before applying if it's not already the case

@sofar
Copy link
Contributor

sofar commented May 3, 2017

Can we just add Set this value to current window size button in Advanced Settings dialog? Like Blender app does.

And enable it by default.

I don't understand why this discussion has 30+ comments on it, this is too stupid not to merge.

And I'm a person who fixes the window to run in 1080p so I can make decent screen recordings, even though my monitor is 2560x1600, so that says a lot.

@rubenwardy
Copy link
Member

I'm fine with this pr as long as it reliably validates the size on start up

@rubenwardy rubenwardy removed the User feedback needed Feedback from players/server owners/modders is needed to determine if the change should be done. label May 3, 2017
@nerzhul nerzhul added this to the 0.4.16 milestone May 4, 2017
@nerzhul
Copy link
Member Author

nerzhul commented May 4, 2017

@rubenwardy I tested in master without my patch (which doesn't affect screenW reading), setting screenW to 7000 into my configuration make irrlicht resize the window to the screen max width

@nerzhul
Copy link
Member Author

nerzhul commented May 4, 2017

The setting has been added, please review @paramat @sofar @Zeno-

src/game.cpp Outdated
// Ensure evaluating settings->getBool after verifying screensize
// First condition is cheaper
if (previous_screen_size != device->getVideoDriver()->getScreenSize() &&
g_settings->getBool("autosave_screensize")) {
Copy link
Member

@SmallJoker SmallJoker May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one indent missing (in guiEngine too)
EDIT: Also needs a check against 0,0, when the window is minimized (on Windows?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows set width/height to 0, 0 when minimized ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(late answer) Yes it does.

Loaded texture: E:/Programme/minetest/games/minimal/menu/icon.png
Resizing window (1013 757)
Resizing window (1044 783)
Ignoring resize operation to (0 0)
Resizing window (1044 783)
Resizing window (964 709)
Ignoring resize operation to (0 0)

src/game.cpp Outdated
if (previous_screen_size != device->getVideoDriver()->getScreenSize() &&
g_settings->getBool("autosave_screensize")) {
const irr::core::dimension2d<u32> &current_screen_size =
device->getVideoDriver()->getScreenSize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it hurt to move this variable definition outside the if, since the screen size is checked anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exact

@paramat
Copy link
Contributor

paramat commented May 4, 2017

No objections, improves the look of the menu too.

@Zeno-
Copy link
Contributor

Zeno- commented May 5, 2017

👍

@nerzhul nerzhul merged commit 21e0a04 into minetest:master May 5, 2017
@nerzhul nerzhul added this to Done in Minetest 0.4.16 May 13, 2017
@nerzhul nerzhul deleted the window_size_save branch June 8, 2017 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet