Abstract Display code out of OS class #23209

wmww · 2018-10-21T22:24:41Z

This is based on #16744 by @hpvb. If there's a better way to architect it, let me know.

… make it possible to overload them separately

wmww · 2018-10-22T03:10:49Z

I have a fix for the windows build issue CI is reporting, but it doesn't seem to be making it into this PR, due possibly to a GItHub outage (see https://status.github.com/messages). When that situation is resolved, I'll take another look.

toger5 · 2018-10-22T07:07:01Z

Nice you adapted the other platforms already! I only adapted osx and ran into problems... What is checked till now? (like actually tested, not only travis)

toger5 · 2018-10-22T07:09:17Z

Nice you already fixed all the plaforms! I only tried osx, but ran into issues.
What platforms are already properly tested? Travis doesn't like anything except Server.

.buildconfig

wmww · 2018-10-22T15:14:12Z

I've only actually tested on Freedesktop (which is all the original code worked with). Server, javascript and android all build, though javascript fails on some final step for me and gradle runs into problems for Android (both issues for me when using master as well, but they prevent me from testing). I'll give updates if I do manage to get those working. Testing would be most appreciated on OSX, IPhone and the windows platforms, as I can't build locally for them at all.

wmww · 2018-10-22T15:31:01Z

Yep, even in the last few minutes my comments and edits have been inconsistent. Looks like they've been working through the night to fix it.

Is there a way I can make CI go again with my changes, or does that happen automatically when GitHub is working?

LikeLakers2 · 2018-10-22T15:33:28Z

(I deleted my last comment because I didn't think it was really needed, woop. So that comment not appearing isn't GitHub's fault. For anyone else reading this, I basically said I'm gonna review the PR once more once GitHub isn't on the fritz.)

You could make another commit, but I'd wait until GitHub's working again.

groud · 2018-10-22T15:38:59Z

I think we do not use multiple inheritance in general. I don't remember why (it might be because it creates problems with the classdb) but the fact that you are using it might be a problem.

wmww · 2018-10-22T15:55:56Z

I believe all the DisplayDriver code is still part of the OS from the classdb's perspective. I agree it's not great and I'm open to suggestions.

I can see three possible alternative ways of doing things:

Split every OS_ class into an OS and a DisplayDriver class
- More code than I particularly want to write
- More code than anyone wants to review
Stop seeing DisplayDriver as a cross-platform concept
- Keep the same OS abstraction
- Have OS_Freedesktop delegate its display driver needs to an abstract class
- Would require this PR to be mostly rewritten, but would not increase its scope
Only implement windowing system agnostic functionality in OS_Freedesktop, and have OS_X11, OS_Wayland, etc. inherit from it
- DisplayDriver abstraction not needed (it's simply whatever OS functions none of the parent classes implement)
- Less boilerplate then previous option (OS_Freedesktop doesn't have to proxy a bunch of function calls on to another class)
- Single inheritance, and keep the same architecture

Edit: added 3rd option

groud · 2018-10-22T16:09:58Z

Well, I might be wrong but the only place where the display abstraction is needed is the freedesktop platform. So, IMHO, there is no real reason to make this abstraction elsewhere unless it clearly make the code easier to read and to use.

Personally, I would keep the API as is for the OS class, and only call an x11 driver class from the freedesktop OS class. (But, to be honest I am not the most qualified one in this part of the code ^^)

Edit: Basically, what I wrote is your solution number two. ^^

wmww · 2018-10-22T17:12:36Z

I added a 3rd option. Ideally I can get input from @toger5, @hpvb, and one of the main Godot devs before rewriting or heavily modifying this PR (if that is what is determined to be required).

toger5 · 2018-10-22T19:12:03Z

I really like the clearity in the API we separate DisplayDriver functions from OS. Although the bahaviour is objectively only needed for freedesktop.
Can someone confirm + explain what problems multi inheritance raises? I think in this case it might be okay since the classes (OS_freedesktop, OS_android ...) are not even exposed to class db only their parent classes are exposed and they only inherit form one/no other class.
If there is an actual issue i highly advocate the second solution (the one @groud also proposed). OS_freedesktop decides which DispDirver to instanciate and than it just calls the DisplayDriver functions:

...
bool OS_freedesktop::is_window_maximized() const {
    display_driver->is_window_maximized();
};
...

Different Topic (Naming conventions):
Call Freedesktop just Linux:
Although freedesktop is very precise in what OS_freedesktop takes care of, it is confusing for ppl who look at the source for the first time. They definitly expect OS_Linux next to OS_OSX and OS_Windows.
Even ppl who dont code c++ but want to compile godot themselves would need to use scons p=freedesktop which is not intuitive (scons p=linux is better imo).
Third point: it will still be called Linux in the export menu the editor. So we also get consensus in that regard.

Is it good design to call classes OS_freedesktop or Display_X11 / Display_wayland?
I dont like the _. Godot uses it in a bunch of places so I guess this is desired. If not, right now would be a good chance to change it.

LikeLakers2 · 2018-10-22T23:15:45Z

COPYRIGHT.txt

@@ -87,9 +87,9 @@ Copyright: 2002, Google Inc.
 License: Apache-2.0

 Files: ./platform/android/power_android.cpp
+ ./platform/freedesktop/power_x11.cpp


Just being a little nitpicky here, but if we're renaming the x11 platform to freedesktop, shouldn't the filenames be changed too?:

LikeLakers2 · 2018-10-22T23:17:25Z

doc/tools/makemd.py

@@ -0,0 +1,360 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-


`makemd.py was removed from master some time ago for being an unused script.

drivers/gles2/rasterizer_gles2.cpp

LikeLakers2 · 2018-10-22T23:24:16Z

drivers/gles3/rasterizer_gles3.cpp

 #include "core/os/os.h"
 #include "core/project_settings.h"
 #include "drivers/gl_context/context_gl.h"
+#include <string.h>


This line was removed from master some time ago.

LikeLakers2 · 2018-10-22T23:25:57Z

core/os/os.h

+	// virtual void set_window_always_on_top(bool p_enabled) {}
+	// virtual bool is_window_always_on_top() const { return false; }
+	// virtual void request_attention() {}
+	// virtual void center_window();


If you're removing all this stuff from os.cpp, I don't see a need to keep commented-out headers.

LikeLakers2 · 2018-10-22T23:39:24Z

modules/gdmir

@@ -0,0 +1 @@
+/home/wmww/code/gdmir/module/


I don't think you meant to commit this file either, nor modules/stytd6U8.

LikeLakers2 · 2018-10-22T23:41:35Z

modules/mobile_vr/shaders/SCsub

@@ -0,0 +1,6 @@
+#!/usr/bin/env python


This file was moved to a different location in master some time ago.

LikeLakers2 · 2018-10-22T23:46:08Z

Sorry for the large number of comments, I believe I'm done reviewing for now. A couple other things I noted:

I noted some files and lines that were removed before on the master branch, that this PR adds back. ~~I think your fork is behind? and Git or GitHub thinks you're trying to re-add the files/lines?~~ Actually, I'm not sure what's going on, after some further investigation. Either way, I've already pointed out the specific lines/files in my comments above.
I noted some files where you add an include despite not making any other changes (such as with crash_handler_x11.cpp). Any reason for that?

(Also, I feel the need to point out that I won't be reviewing the code itself to make sure it works, or commenting on any design choices -- I'm leaving that up to other, more knowledgeable people. I'm simply pointing out the little things that I noticed, as my many comments above should show. So please, take all of this as you will. :) )

Co-Authored-By: wmww <[email protected]>

wmww · 2018-10-23T13:10:14Z

Thanks! All those comments seem valid. I'll go through and fix them one by one, but only if others decide its a good idea to go forward with the current design. Otherwise, many of them may not be relevant (or I might just end up rewriting the whole thing).

toger5 · 2018-10-24T10:35:40Z

Update: @wmww and me had a call. Conclusion: we should use option 3 where os_wayland and os_x11 are different classes both inheriting from os_linux (os_freedesktop) We check if a connection to a wayland composition is possible than instance the os_wayland class otherwise it will run on x11.

advantages:

the os api does not get changed -> no changes to other classes
there is still a separation for linux display and other os stuff. in os_linux.cpp we have all the shared implementations and in os_wayland only the wayland specific stuff (since it inherits from os_linux...)
much smaller diff
no performance hit since the check if it runs on wayland is really fast/simple:

display = wl_display_connect(NULL);
if (display == NULL) {
    NO WAYLAND
}
WAYLAND

than the correct os gets initialised...

toger5 · 2018-10-31T00:36:39Z

I would like to implement this abstraction, by just making two seperated classes. (as i explained in the last post) (summerized: No DisplayDriver class/singleton)

I just need green light, to be sure im not overlooking anything.

wmww · 2018-10-31T20:20:27Z

Moving to #23425

wmww requested review from BastiaanOlij, leonkrause, karroffel, reduz and vnen as code owners October 21, 2018 22:24

wmww changed the title ~~Display abstraction 2~~ Abstract Display code out of OS class Oct 21, 2018

wmww force-pushed the display-abstraction-2 branch from bdb21c7 to 943baa2 Compare October 21, 2018 22:28

wmww added 10 commits October 21, 2018 15:34

Split display code out of OS class

299031a

Add _os and _display suffixes to initialize and finalize functions to…

6c28e50

… make it possible to overload them separately

Update OS_Server

88d8dc5

Update OS_Haiku for OS and DisplayDriver (don't fix existing problems)

d9d6642

Update OS_JavaScript

802ca2a

Update OS_Android

024a2c1

Update OS_OSX

b113217

update OSIPhone

ad5a2e9

Update OSUWP

a79be6a

Update OS_Windows

55a8bfb

wmww force-pushed the display-abstraction-2 branch from 943baa2 to 55a8bfb Compare October 21, 2018 22:34

Fix get_video_driver_name for Windows

1c16638

akien-mga added feature proposal topic:core topic:porting labels Oct 22, 2018

akien-mga added this to the 3.2 milestone Oct 22, 2018

LikeLakers2 reviewed Oct 22, 2018

View reviewed changes

.buildconfig Outdated Show resolved Hide resolved

Remove accidentally committed .buildconfig

2159c9f

groud requested a review from hpvb October 22, 2018 16:10

LikeLakers2 reviewed Oct 22, 2018

View reviewed changes

drivers/gles2/rasterizer_gles2.cpp Outdated Show resolved Hide resolved

LikeLakers2 reviewed Oct 22, 2018

View reviewed changes

Update drivers/gles2/rasterizer_gles2.cpp

f1110e2

Co-Authored-By: wmww <[email protected]>

wmww closed this Oct 31, 2018

toger5 mentioned this pull request Nov 1, 2018

abstract Display functionality (step one for wayland) RENAME x11 -> linux #23425

Closed

akien-mga added the archived label Nov 18, 2018

toger5 mentioned this pull request Nov 25, 2018

[WIP] Introduce OS.set_drag_mode() - Tell the window manager to move / resize the window. #16512

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract Display code out of OS class #23209

Abstract Display code out of OS class #23209

wmww commented Oct 21, 2018 •

edited

Loading

wmww commented Oct 22, 2018

toger5 commented Oct 22, 2018

toger5 commented Oct 22, 2018

wmww commented Oct 22, 2018 •

edited

Loading

wmww commented Oct 22, 2018

LikeLakers2 commented Oct 22, 2018

groud commented Oct 22, 2018

wmww commented Oct 22, 2018 •

edited

Loading

groud commented Oct 22, 2018 •

edited

Loading

wmww commented Oct 22, 2018

toger5 commented Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 commented Oct 22, 2018 •

edited

Loading

wmww commented Oct 23, 2018 •

edited

Loading

toger5 commented Oct 24, 2018 •

edited

Loading

toger5 commented Oct 31, 2018

wmww commented Oct 31, 2018

		@@ -0,0 +1,360 @@
		#!/usr/bin/env python
		# -- coding: utf-8 --

		@@ -0,0 +1 @@
		/home/wmww/code/gdmir/module/

Abstract Display code out of OS class #23209

Abstract Display code out of OS class #23209

Conversation

wmww commented Oct 21, 2018 • edited Loading

wmww commented Oct 22, 2018

toger5 commented Oct 22, 2018

toger5 commented Oct 22, 2018

wmww commented Oct 22, 2018 • edited Loading

wmww commented Oct 22, 2018

LikeLakers2 commented Oct 22, 2018

groud commented Oct 22, 2018

wmww commented Oct 22, 2018 • edited Loading

groud commented Oct 22, 2018 • edited Loading

wmww commented Oct 22, 2018

toger5 commented Oct 22, 2018 • edited Loading

LikeLakers2 Oct 22, 2018

Choose a reason for hiding this comment

LikeLakers2 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

LikeLakers2 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

LikeLakers2 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

LikeLakers2 Oct 22, 2018

Choose a reason for hiding this comment

LikeLakers2 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

LikeLakers2 commented Oct 22, 2018 • edited Loading

wmww commented Oct 23, 2018 • edited Loading

toger5 commented Oct 24, 2018 • edited Loading

toger5 commented Oct 31, 2018

wmww commented Oct 31, 2018

wmww commented Oct 21, 2018 •

edited

Loading

wmww commented Oct 22, 2018 •

edited

Loading

wmww commented Oct 22, 2018 •

edited

Loading

groud commented Oct 22, 2018 •

edited

Loading

toger5 commented Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 Oct 22, 2018 •

edited

Loading

LikeLakers2 commented Oct 22, 2018 •

edited

Loading

wmww commented Oct 23, 2018 •

edited

Loading

toger5 commented Oct 24, 2018 •

edited

Loading