ashea-code / BLUI

Rich HTML UI engine for UE4
MIT License
863 stars 259 forks source link

File organization issues #8

Closed gmpreussner closed 9 years ago

gmpreussner commented 9 years ago

Hey Aaron,

I stumbled upon your project through your video on YouTube and very much like what you're doing here. Peeking at the code, I noticed a few problems with how you organize your files, so I'd like to throw some suggestions your way:

1) The 'Classes' folder is obsolete and no longer required. You can now put UObject declarations into Public and Private

2) Your public header files in Classes include BluPrivatePCH.h. This is not desirable, because that forces your users to include a private header. By definition, the "Private Pre-Compiled Header" file should only be used internally and not be accessible outside your module.

3) BluPrivatePCH.h should be included only by your .cpp files, not any of the private or public header files. If your private/public header files require other headers in your module, then you should include those header files explicitly.

4) In general, you should minimize the number of other header files that any given header file depends on. Ideally, no header file should include any other header files. This can almost always be accomplished by using forward declaration of types. One of the common exceptions is when you derive from an external type, i.e. IModuleInterface. Note that most types declared in Core do not require explicit inclusion as the majority of Core headers are always included automatically.

5) You may want to limit your plug-in to those platforms that actually support and are set up to compile CEF by white-listing them in your .uplugin file.

I'm looking forward to seeing how you send JavaScript events back to UE4. Some Blueprint integration would be nice. Good luck :)

ashea-code commented 9 years ago

Hello! Thank you very much for giving some advice, it's nice to hear from someone at EPIC. The UI plugin has mostly been a side project of mine, up until now I've been making it up as it goes along. However that changed when I saw this could become a very helpful system.

I will work on reorganizing the header files (mostly removing the PCH from Classes header files. I'll be doing some testing on other platforms including Linux. (Testing OSX will be difficult as I don't have an Apple computer at my disposal) But I will white-list those that are supported.

Passing JS events back into UE4 works perfectly in tandem with the custom sub-process (https://github.com/AaronShea/BluBrowser) which will be packaged with releases here on GitHub. But the feature is currently undocumented. You can call blu_event(event_name, event_data) to send a JS event back into UE4 (see https://github.com/AaronShea/BLUI/blob/master/Source/Blu/Classes/BluWidget.h#L81)

Thanks for all the feedback!

ashea-code commented 9 years ago

You wouldn't happen to know the uplugin syntax for platform white listing would you? I can't find it documented anywhere. I did add checks in the build cs file. @gmpreussner

gmpreussner commented 9 years ago

Hey Aaron, the settings go into the "Modules" section as "WhitelistPlatforms" and/or "BlacklistPlatforms". For example:

{
    // other settings here

    "Modules" :
    [
        {
            "Name" : "MyPlugin",
            "BlacklistPlatforms" : [ "HTML5" ]
        }
    ]
}

If you search for those keywords in *.uplugin files, you should be able to find more examples.

gmpreussner commented 9 years ago

Yes, looking much better :) One more suggestion.... this:

#include "../Public/BluInstance.h"

...can be simplified to:

#include "BluInstance.h"

... because all sub-directories in the Public directory are discovered automatically by UBT.

ashea-code commented 9 years ago

@gmpreussner oddly enough, when I change this, Visual Studio (and the compiler) complains about not being able to find these header files.

gmpreussner commented 9 years ago

Hmm... that is rather unexpected. A module should always find its own Public header files. I wonder if something in your Build.cs breaks the logic. I took a quick look and couldn't see anything obvious, but something must be wrong somewhere. We're not using relative include paths for module headers anywhere in the engine or in plug-ins :/

ashea-code commented 9 years ago

@gmpreussner looked into it, seems I did break something in the build.cs, all fixed now with the above commit, thanks for the help!