Worked with Faless(Fabio Alessandrelli) to update server platform. #16653

fire · 2018-02-13T01:03:28Z

It's an updated version of the server platform. Uses dummy for the driver name. (Any name changes can be discussed elsewhere.)

Add dummy rasterizer driver.

Disable GLES builders and source from server compilation.

Add dummy texture handling.

Server platform now compiles and run on linux.

Seems to also be able to do exports of some demos I tried.

Fixes to OS_Server and DummyRasterizer to match new signatures.

Add dummy audio driver.

Update to newer api.

Faless · 2018-02-13T02:10:30Z

I've rebased your changes on top of my commits so It's easier to review, and nicer for the commit log.
master...Faless:server_again_ifire (you can fetch it and for push it).
Additionally, I would discourage the usage of the override keyword as there is no need for it.

fire · 2018-02-14T00:37:02Z

Looking for review and merge.

akien-mga · 2018-02-14T10:20:06Z

I don't understand, what branch should be considered for merging? ifire:server_again_2 from this PR or Faless:server_again_ifire linked by @Faless?

akien-mga · 2018-02-14T10:20:52Z

drivers/dummy/rasterizer_dummy.h

@@ -0,0 +1,539 @@
+/*************************************************************************/
+/*  rasterizer.h                                                         */


Should be rasterizer_dummy.h (with the ending */ properly aligned after the edit).

akien-mga · 2018-02-14T10:21:06Z

drivers/dummy/SCsub

+
+Import('env')
+
+env.add_source_files(env.drivers_sources,"*.cpp")


Missing space after , to respect PEP8.

akien-mga · 2018-02-14T10:21:13Z

drivers/dummy/rasterizer_dummy.h

+/*                           GODOT ENGINE                                */
+/*                      https://godotengine.org                          */
+/*************************************************************************/
+/* Copyright (c) 2007-2017 Juan Linietsky, Ariel Manzur.                 */


Nevermind that's fixed in a later commit.

akien-mga · 2018-02-14T10:23:02Z

platform/server/SCsub

@@ -5,6 +5,8 @@ Import('env')

 common_server = [\
    "os_server.cpp",\
+    "#platform/x11/crash_handler_x11.cpp",


Waiting for linuxbsd platform to cleanup this mess :P

akien-mga · 2018-02-14T10:24:38Z

drivers/dummy/audio_driver_dummy.h

@@ -0,0 +1,58 @@
+/*************************************************************************/
+/*  audio_driver_dummy.h                                            */


Ending */ should be aligned with the rest.

akien-mga · 2018-02-14T10:25:56Z

Looking for review and merge.

Need an answer to @Faless' comment before that:

Additionally, I would discourage the usage of the override keyword as there is no need for it.

fire · 2018-02-14T15:28:48Z

Personal opinion, override (c++11) should be used whenever implementing a virtual function. It uses the compiler to help detect if the method made is the exact method overridden.

But consistency in the engine source is a good goal too.

akien-mga · 2018-02-14T15:31:37Z

drivers/dummy/rasterizer_dummy.h

@@ -39,584 +39,584 @@

 class RasterizerSceneDummy : public RasterizerScene {
 public:
-	/* SHADOW ATLAS API */
+    /* SHADOW ATLAS API */


Something went wrong here. IDE misconfiguration, or wrong version of clang-format? (We need 5.0).

Once fixed, please squash the fixups into the last commit, as we don't really need a specific commit for those.

Visual studio code wants to reformat everything, while msvc ide has my clang-format settings.

Faless · 2018-02-14T15:32:48Z

We still have to decide the fate and rules for c++11 in the Engine, I think we can add it later when those rules are set.

…

On Feb 14, 2018 16:29, "K. S. Ernest (iFire) Lee" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In drivers/dummy/rasterizer_dummy.h <#16653 (comment)>: > @@ -39,584 +39,584 @@ class RasterizerSceneDummy : public RasterizerScene { public: - /* SHADOW ATLAS API */ + /* SHADOW ATLAS API */ Something went wrong here. IDE misconfiguration, or wrong version of clang-format? (We need 5.0). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16653 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnBbquC6SD6hiw7_V_VoFygmeFzxeJsks5tUvxygaJpZM4SDENV> .

akien-mga · 2018-02-14T15:32:57Z

But consistency in the engine source is a good goal too.

For now consistency is more important, we shouldn't start doing such changes in some places based on personal preference.
If there's a reason to use those everywhere consistently, it should be discussed, agreed upon and added all at once.

fire · 2018-02-14T15:36:48Z

drivers/dummy/rasterizer_dummy.h

+    void lightmap_capture_set_energy(RID p_capture, float p_energy) {}
+    float lightmap_capture_get_energy(RID p_capture) const { return 0.0; }
+    const PoolVector<RasterizerStorage::LightmapCaptureOctree> *lightmap_capture_get_octree_ptr(RID p_capture) const {
+        PoolVector<RasterizerStorage::LightmapCaptureOctree> p;


Is there memory leak / memory allocation bug here?

fire · 2018-02-14T15:55:25Z

Submitted the whitespace changes, removed override and added a comment on a pointer return that looks suspicious.

Faless · 2018-02-14T15:56:26Z

drivers/dummy/rasterizer_dummy.h

+	float lightmap_capture_get_energy(RID p_capture) const { return 0.0; }
+	const PoolVector<RasterizerStorage::LightmapCaptureOctree> *lightmap_capture_get_octree_ptr(RID p_capture) const {
+		PoolVector<RasterizerStorage::LightmapCaptureOctree> p;
+		return &p;


Is there memory leak / memory allocation bug here?

No, but it returns an invalid pointer that might crash the engine or corrupts its memory if used

How would you fix it?

Not sure, probably the safest way is to implement lightmap storage properly, like I did for texture (i.e. implement create, owner, and the like)

fire · 2018-02-15T03:26:15Z

@Faless how does this look?

fire · 2018-02-15T15:12:06Z

Tried to export a project on WSL (Windows 10)'s Ubuntu.

$ ../fire-godot/bin/godot_server.server.tools.64 --export "Windows Desktop" ../Pong.exe
ERROR: initialize: AudioDriverManager: all drivers failed, falling back to dummy driver
   At: servers/audio_server.cpp:153.
ERROR: create: Cannot find path for config directory!
   At: editor/editor_settings.cpp:771.
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7f6e2ad754b0] (??:0)
[2] ../fire-godot/bin/godot_server.server.tools.64() [0xd03853] (/mnt/d/Projects/fire-godot/editor/editor_file_system.cpp:1047)
[3] ../fire-godot/bin/godot_server.server.tools.64() [0xcfe067] (/mnt/d/Projects/fire-godot/editor/editor_file_system.cpp:279)
[4] ../fire-godot/bin/godot_server.server.tools.64() [0xcfe642] (/mnt/d/Projects/fire-godot/editor/editor_file_system.cpp:298)
[5] ../fire-godot/bin/godot_server.server.tools.64() [0xc6077d] (/mnt/d/Projects/fire-godot/drivers/unix/thread_posix.cpp:72)
[6] /lib/x86_64-linux-gnu/libpthread.so.0(+0x76ba) [0x7f6e2bbe76ba] (??:0)
[7] /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x7f6e2ae473dd] (??:0)
-- END OF BACKTRACE --

fire · 2018-02-15T15:25:36Z

virtual bool has_environment(const String &p_var) const;
virtual String get_environment(const String &p_var) const;
virtual String get_locale() const;

virtual int get_processor_count() const;

Aren't implemented.

[Actually, don't know. These are inherited from OS_unix.]

Seems to also be able to do exports of some demos I tried.

fire · 2018-02-15T16:16:11Z

https://github.com/godotengine/godot/blob/master/editor/editor_file_system.cpp#L278

Removing these lines allowed the export to finish.

f->close();
memdelete(f);

fire · 2018-02-15T16:20:53Z

diff --git a/editor/editor_file_system.cpp b/editor/editor_file_system.cpp
index dca32d749..1251a0e56 100644
--- a/editor/editor_file_system.cpp
+++ b/editor/editor_file_system.cpp
@@ -276,8 +276,6 @@ void EditorFileSystem::_scan_filesystem() {
 
 	f = FileAccess::open(fscache, FileAccess::WRITE);
 	_save_filesystem_cache(new_filesystem, f);
-	f->close();
-	memdelete(f);
 
 	scanning = false;
 }
@@ -1044,6 +1042,10 @@ void EditorFileSystem::_save_filesystem_cache(EditorFileSystemDirectory *p_dir,
 
 	if (!p_dir)
 		return; //none
+
+	if (!p_file)
+		return; //none
+
 	p_file->store_line("::" + p_dir->get_path() + "::" + String::num(p_dir->modified_time));
 
 	for (int i = 0; i < p_dir->files.size(); i++) {

This is not correct, but the problem is around here.

akien-mga · 2018-02-15T16:26:02Z

Well your problem is:

ERROR: create: Cannot find path for config directory!
   At: editor/editor_settings.cpp:771.

Then all files it tries to open will be invalid, and if there aren't null checks (like here), it will crash. But that's likely unrelated to this PR and just related to caveats of running Linux binaries on WSL.

neikeq · 2018-09-24T16:26:24Z

platform/server/detect.py

@@ -31,6 +28,7 @@ def get_opts():
 def get_flags():

    return [
+            ("module_mobile_vr_enabled", False),


Why are we doing this? There is a problem here. The mobile_vr module registers the class MobileVRInterface. By only enabling it on some platforms, this breaks API portability. This is specially harmful for GDNative and C#.

The code that registers the class is the following:

godot/modules/mobile_vr/register_types.cpp

Lines 35 to 41 in c432ce4

void register_mobile_vr_types() {

ClassDB::register_class<MobileVRInterface>();

Ref<MobileVRInterface> mobile_vr;

mobile_vr.instance();

ARVRServer::get_singleton()->add_interface(mobile_vr);

}

I can imagine three different situations and the possible solutions:
Can the MobileVRInterface class be used directly, or can it only be used through the ARVRInterface interface?

If the MobileVRInterface class cannot be used directly from scripts and scripts only use the ARVRInterface interface then this class must not be registered manually. register_class will expose the class to the scripting API, which must be portable. When you create an instance, the class will be registered automatically without exposing it to the scripting API.

If MobileVRInterface or any of its methods from that are not registered by ARVRInterface can be used directly from scripts, then we have a different problem. One of the possible solution could be to create a dummy class.

What confuses me the most though is that this class is not actually disabled on the desktop, only on server builds. I think this is because of the gl code in this class, am I right? If that is the case, then perhaps the solution is to surround such parts of the code with #ifdefs and maybe to not call ARVRServer::get_singleton()->add_interface(mobile_vr) either.

If you missed the irc conversation, fales replied.

<fales> neikeq seems like VR module uses GLES header

<fales> which is of course not avail in server platform

<fales> it uses LensDistortedShaderGLES3 which is defined "shaders/lens_distorted.glsl.gen.h" maybe we could guard the functions by defines. Not sure what the best solution is. There's also a comment from Mux213 about that shader and its todo state for GLES2 support. Not sure if anything changed

That would be the third possibility I mentioned.
Can't we ifdef that code or add a dummy class?

@neikeq yes to both.
I think we should ask @BastiaanOlij about the GLES2 comment.
He mentions some refactoring about the shader there.

godot/modules/mobile_vr/mobile_vr_interface.cpp

Lines 300 to 305 in 4226d56

// build our shader

if (lens_shader == NULL) {

///@TODO need to switch between GLES2 and GLES3 version, Reduz suggested moving this into our drivers and making this a core shader

// create a shader

lens_shader = new LensDistortedShaderGLES3();

In the meantime I'll see if we can have a dummy implementation of the interface.

I hope to finally have time for this on my trip to Europe next week.

That said, for the server platform, why not change the detect.py so the module is not included on the server installation? VR is not really applicable on the server so..

@BastiaanOlij we're already doing that right now, and that's the problem. The mobile_vr module exposes the class MobileVRInterface to the scripting API. By disabling the module the scripting API for server builds becomes different to that of other target platforms. That breaks API portability. This hurts specially GDNative and C#, which rely on generating bindings.
Also doesn't this break GDScript scripts that make use MobileVRInterface as well?

Also doesn't this break GDScript scripts that make use MobileVRInterface as well?

Yes, for people who uses the server platform for exporting the game (e.g. in CI).

BastiaanOlij · 2018-09-26T06:25:55Z

Ok, fair enough. I do plan to spend time on this next week, get it going on Gles2, that may involve moving this into the render drivers after which it can be just an empty method on the dummy driver.

On Wed, 26 Sep 2018 at 11:29 am, Fabio Alessandrelli < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In platform/server/detect.py <#16653 (comment)>: > @@ -31,6 +28,7 @@ def get_opts(): def get_flags(): return [ + ("module_mobile_vr_enabled", False), Also doesn't this break GDScript scripts that make use MobileVRInterface as well? Yes, for people who uses the server platform for exporting the game (e.g. in CI). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16653 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2vadxo3DFRZFGOUJDjGtbBbAoCzcUdks5uetiTgaJpZM4SDENV> .

-- Kindest regards, Bastiaan Olij https://www.facebook.com/bastiaan.olij https://twitter.com/mux213 https://www.youtube.com/channel/UCrbLJYzJjDf2p-vJC011lYw https://github.com/BastiaanOlij

neikeq · 2018-09-26T15:48:37Z

Meanwhile, do you think #22422 can be a good temporary solution?

akien-mga · 2018-09-28T07:39:17Z

I merged #22422 for now as it's an OK workaround for the issue until something better can be implemented. It might be worth it to open a new issue to keep track of what needs to be done (and remember to revert/undo #22422 at that time).

fire force-pushed the server_again_2 branch from 75c0ef2 to 0b2c930 Compare February 13, 2018 01:12

fire force-pushed the server_again_2 branch from 0b2c930 to 4551a01 Compare February 13, 2018 03:59

groud added enhancement topic:network labels Feb 13, 2018

akien-mga added this to the 3.1 milestone Feb 14, 2018

akien-mga reviewed Feb 14, 2018

View reviewed changes

fire commented Feb 14, 2018

View reviewed changes

fire force-pushed the server_again_2 branch from c56fa8e to 8d21402 Compare February 14, 2018 15:38

Faless reviewed Feb 14, 2018

View reviewed changes

fire force-pushed the server_again_2 branch 2 times, most recently from 3cb632f to 70a6b77 Compare February 15, 2018 04:02

Faless added 3 commits February 15, 2018 07:34

Add dummy resterizer driver

c0dce6e

Disable GLES builders and source from server compilation

8699f64

Add dummy Texture handling

72ef766

Faless and others added 4 commits February 15, 2018 07:34

server platform now compiles and run on linux.

6784d74

Seems to also be able to do exports of some demos I tried.

Fixes to OS_Server and DummyRasterizer to match new signatures

2de10aa

Add dummy audio driver, fix dummy rasterizer

4e1923a

Create and return LightmapCapture.

2e66730

fire force-pushed the server_again_2 branch from 70a6b77 to 2e66730 Compare February 15, 2018 15:38

akien-mga merged commit da612c3 into godotengine:master Feb 16, 2018

fire deleted the server_again_2 branch February 16, 2018 16:04

karroffel mentioned this pull request Feb 18, 2018

Figure out how to test the bindings. godot-rust/gdnative#10

Closed

hpvb added cherrypick and removed cherrypick labels Feb 20, 2018

Faless mentioned this pull request Mar 16, 2018

Server platform doesn't build #8361

Closed

neikeq reviewed Sep 24, 2018

View reviewed changes

Faless mentioned this pull request Sep 25, 2018

Server platform builds (dummy) mobile_vr module. #22422

Merged

		@@ -0,0 +1,539 @@
		/*************************************************************************/
		/* rasterizer.h */


		Import('env')

		env.add_source_files(env.drivers_sources,"*.cpp")

		@@ -0,0 +1,58 @@
		/*************************************************************************/
		/* audio_driver_dummy.h */

	void register_mobile_vr_types() {
	ClassDB::register_class<MobileVRInterface>();

	Ref<MobileVRInterface> mobile_vr;
	mobile_vr.instance();
	ARVRServer::get_singleton()->add_interface(mobile_vr);
	}

	// build our shader
	if (lens_shader == NULL) {
	///@TODO need to switch between GLES2 and GLES3 version, Reduz suggested moving this into our drivers and making this a core shader
	// create a shader
	lens_shader = new LensDistortedShaderGLES3();

Worked with Faless(Fabio Alessandrelli) to update server platform. #16653

Worked with Faless(Fabio Alessandrelli) to update server platform. #16653

Conversation

fire commented Feb 13, 2018 • edited Loading

Faless commented Feb 13, 2018 • edited Loading

fire commented Feb 14, 2018

akien-mga commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Feb 14, 2018

fire commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faless commented Feb 14, 2018 via email • edited Loading

akien-mga commented Feb 14, 2018

Choose a reason for hiding this comment

fire commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fire commented Feb 15, 2018

fire commented Feb 15, 2018 • edited Loading

fire commented Feb 15, 2018 • edited Loading

fire commented Feb 15, 2018

fire commented Feb 15, 2018

akien-mga commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neikeq Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Faless Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BastiaanOlij commented Sep 26, 2018 via email

neikeq commented Sep 26, 2018

akien-mga commented Sep 28, 2018

fire commented Feb 13, 2018 •

edited

Loading

Faless commented Feb 13, 2018 •

edited

Loading

Faless commented Feb 14, 2018 via email •

edited

Loading

fire commented Feb 15, 2018 •

edited

Loading

fire commented Feb 15, 2018 •

edited

Loading

neikeq Sep 25, 2018 •

edited

Loading

Faless Sep 25, 2018 •

edited

Loading