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

Update main.* in examples to have return values and updated syntax. #4677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bakercp
Copy link
Member

@bakercp bakercp commented Dec 17, 2015

Currently main.* in most examples (with the exception of a few iOS examples) don't actually have a return statement. This hand-crafted (not grepp'ed :)) PR updates all main.cpp example files to fix this.

  • Attention was paid to keep example window sizes the same.
  • Attention was paid to keep any special settings the same.

The updated syntax used is reflected in the new main.* templates here: #4675.

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

One small question -- in gl/oFMaskTool and gl/vboMeshDrawInstancedExample, an older syntax for setting up the programmable renderer is used.

Perhaps this should also be updated to the newer syntax by selecting a specific opengl version?

@danoli3
Copy link
Member

danoli3 commented Dec 17, 2015

Nice work @bakercp 👍 !!!

@arturoc
Copy link
Member

arturoc commented Dec 17, 2015

thanks!

yes any reference to the programmable renderer won't work any more and it should be called using the new syntax. also we should change every main to setup the window using the window settings, that's the most important change. the standard main should look like:

#include "ofApp.h"
#include ofMain.h

//---------------------------------------------------
int main(){
    ofWindowSettings settings;
    settings.width = 1024;
    settings.height = 768;
    settings.windowMode = OF_WINDOW;

    ofCreateWindow(settings);

    ofRunApp(std::make_shared<ofApp>());
}

@arturoc arturoc added this to the 0.10.0 milestone Dec 17, 2015
@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

So -- the standard main you have referenced above -- seems to be a little different than previously discussed as it doesn't have a return value and includes ofMain.h rather than the more minimal ofAppRunner.h -- should this instead be :

#include "ofAppRunner.h"
#include "ofApp.h"

int main(){
    ofWindowSettings settings;
    settings.width = 1024;
    settings.height = 768;
    settings.windowMode = OF_WINDOW;
    ofCreateWindow(settings);
    return ofRunApp(std::make_shared<ofApp>());
}

@arturoc
Copy link
Member

arturoc commented Dec 17, 2015

yeah sorry i was missing the return, i'm not 100% sure about not including ofMain. main is usually fast to compile so including ofMain there shouldn't be such a problem and it'll allow people to use special windows, check for files existing... or any other more complex config in main.

also better to include ofApp.h before ofMain.h/ofAppRunner.h that way ofApp doesn't get all of ofMain.h already included which might clash in some cases

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

Also, this seems like a separate issue -- but it seems inconsistent to have a version of ofRunApp that starts the main loop and one that doesn't

...
return ofRunApp(std::make_shared<ofApp>());

vs.

...
    auto window = ofCreateWindow(settings);
    auto app = std::make_shared<ofApp>();

    ofRunApp(window,app); // doesn't return anything

    return ofRunMainLoop();

If I understand -- the reason for this is to allow multiple windows to be started ...

e.g.

    ofRunApp(guiWindow, guiApp);
    ofRunApp(mainWindow, mainApp);

    return ofRunMainLoop();

but perhaps a better solution for multi-window would be to have a different function for attaching windows to apps so that the api is a little cleaner?

@arturoc
Copy link
Member

arturoc commented Dec 17, 2015

yes i noticed that during a workshop somedays ago, we could just create the mainloop explicitly in that case something like:

...
auto mainLoop = ofGetMainLoop();
mainLoop.addWindowApp(mainWindow, mainApp);
mainLoop.addWindowApp(guiWindow, guiApp);

return mainLoop.run();

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

+1 to a solution that removes void ofRunApp(window,app);

and does this look like an appropriate default main.* file?

#include "ofApp.h"
#include "ofMain.h"

int main(){
    ofWindowSettings settings;
    settings.width = 1024;
    settings.height = 768;
    settings.windowMode = OF_WINDOW;
    ofCreateWindow(settings);
    return ofRunApp(std::make_shared<ofApp>());
}

If so, I'll make changes in the PR and also update the templates in the other PR to remove the use of the older ofSetupOpenGL(...) and deprecate it :)

@arturoc
Copy link
Member

arturoc commented Dec 17, 2015

yes that should be it. i have a couple of doubts about what should be the default now :) if we are going to change the syntax for multiwindow, it probably makes sense to have the same syntax for all the applications so it's easier to go from one -> several so perhaps we should use:

#include "ofApp.h"
#include "ofMain.h"

int main(){
    ofWindowSettings settings;
    settings.width = 1024;
    settings.height = 768;
    settings.windowMode = OF_WINDOW;

    auto window = ofCreateWindow(settings);
    auto app = std::make_shared<ofApp>();
    auto mainLoop = ofGetMainLoop();

    mainLoop.add(window, app);
    return mainLoop.run();
}

? it's way more verbose but it's easier to go from there to multiwindow.

kind of the same happens with setting the GL version or other settings specific to the platform window. we now have:

  • ofWindowSettings the most generic, can setup any platform
  • ofGLWindowSettings -> generic desktop can setup GL version
  • ofGLESWindowSettings -> generic mobile can setup GLES version
  • ofGLFWWindowSettings -> specific to GLFW can set things like minified, no decorations...
  • ... (other platform windows)

perhaps we should discuss a little bit about this to try what would be a default option that makes it easier to use the most advanced uses but still is multiplatform...

anyway let's try to find the best solution mostly so you don't have to change the files multiple times

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

Personally, and from a teaching perspective, making things multi-platform and verbose is better. I would rather use (and teach) a more verbose, step by step approach (like what you propose) than hiding everything at the window configuration stage inside of a mysterious ofRunApp function. Like the iOS examples, it gives users an opportunity to discover that there are all sorts of configuration options for windows available to them. It also makes it much easier to "add" windows, which is usually something you do after starting with one window model and realizing that maybe it would be cool to add another one ... :) so making that addition just an extra line or two by default would be really nice.

That said, I think we should get some other voices in on this conversation ... @danoli3 @digitalcoleman -- what do you think?

@ofTheo
Copy link
Member

ofTheo commented Dec 17, 2015

I also prefer showing the options available.
Right now ofMain can be super mysterious - I think showing more of the options ( even if some are commented out ) is really helpful to beginners.

I also think a slightly less specific way to set the programmable renderer would be useful.
Where you don't need to specify a GL version - but thats a separate issue. :)

@digitalcoleman
Copy link
Contributor

I would agree that having this be more verbose will encourage more creative solutions, and additionally, it is not so verbose that it is indecipherable.
Right now in the classroom the refrence to the main file goes something like "thats where you change the window size and you can do other stuff later when you are more advanced so dont mess with anything else"
The new proposals above are actually teachable as to how and why they are needed and function. Frankly, I had to have a CS student explain fully to me what that file was for....

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

Seems like a pretty good consensus that a more verbose, flexible main.cpp. I think in order to do this then, the next step is to update the core with the additional ofMainLoop syntax that will allow something like this:

#include "ofApp.h"
#include "ofMain.h"

int main(){
    ofWindowSettings settings;
    settings.width = 1024;
    settings.height = 768;
    settings.windowMode = OF_WINDOW;

    auto window = ofCreateWindow(settings);
    auto app = std::make_shared<ofApp>();
    auto mainLoop = ofGetMainLoop();

    mainLoop->addWindow(window, app);
    return mainLoop->run();
}

There is already a method called addWindow -- but I believe that since it's adding a window AND and app, perhaps it should be called something else? Maybe addWindowWithApp(...). I think ultimately it just needs to be clear for those who are using multiwindow with / without multiple apps.

@bakercp
Copy link
Member Author

bakercp commented Dec 17, 2015

Or, perhaps it would be better to have addAppWithWindow(...,...) or just addApp(...)

@bakercp
Copy link
Member Author

bakercp commented Feb 2, 2016

@arturoc let me know when you get the syntax you suggested updated and I can update this PR.

@bakercp
Copy link
Member Author

bakercp commented Apr 15, 2018

@kylemcdonald @arturoc @ofTheo I'm happy to bring this up to date if it will be merged.

Also happy to sync it with this one.

#4675

Let me know.

@kylemcdonald
Copy link
Contributor

this is way more comprehensive than what i was planning on doing... :) i hope this makes it in. i have a feeling 0.10.0 is going to be around for a while.

@arturoc
Copy link
Member

arturoc commented Apr 16, 2018

I'm not really sure about a couple of things regarding which function calls we want to use, in the future we should move all the examples to use ofGLSettings but because now there's ofGLSettings and ofGLESSettings we would need ifdefs in the main files so they would be multiplatform which i don't like much.

Probably the solution is to join those two structs into one. We also have ofGLFWWindowSettings which i guess people can just opt in if they want more specific features

Apart from that using a shared_ptr for the app was probably not a very good idea and i think we should remove it and at most use a unique pointer or just a plain allocation as we do now that will be turned into a unique ptr internally.

So can you include ofMain instead of ofAppRunner so people can use different window settings without having to know the specific header and remove the shared pointer? i think we can merge this as it is and leave the settings setup for next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants