Skip to content

Conversation

@d235j
Copy link
Contributor

@d235j d235j commented Nov 7, 2015

This enables automatic searching for config files based on a list of hardcoded paths (this can be changed if needed), or loads a default empty config if no config filename is passed.

This, together with #264, should solve #238, #185, and #171, #172, at least, but may need some changes/tweaks.

Before we can claim full autoconfig support we also need to implement plugin directory path detection.

@rpavlik
Copy link
Member

rpavlik commented Nov 13, 2015

Following up from discussion on #271 (moving discussion here where it fits better):

The displays directory is used in osvr_server config files: for example, this one https://github.com/OSVR/OSVR-Core/blob/master/apps/sample-configs/osvr_server_config.dSight.json (The default is to prepopulate the data structure with a compiled-in version of the HDK display descriptor, so if you don't specify one explicitly that's what it uses.)

The "displays" entry can either contain the full display descriptor json object right there, or it can be a filename or a (limited subset of valid forms of) JSON reference. The code that handles that is in the following files, designed/coded with the knowledge it would probably expand, move into common or util, and gain awareness of some sense of "json search path" because right now it's pretty fragile and primarily current-working-directory-dependent (which happens to work OK in the case of Windows where working dir defaults to executable dir if you double-click it).

used for displays here: https://github.com/OSVR/OSVR-Core/blob/master/src/osvr/Server/ConfigureServer.cpp#L345
(It's used in a few other places in the server configuration procedure, like resolving device descriptors for external VRPN devices, a potentially separate rendermanager config file, etc.)

Eventually, of course, many times these display descriptors will be compiled in to plugins and auto-detected, but there are plenty of cases/reasons when display parameters couldn't be autodetected or don't need a full plugin, so this configuration option is likely to stick around even once display autodetect functionality is in there.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file needs to be generated - it doesn't look like it's using any CMake substitutions. Just consuming #ifdefs from a configured header is fine in a regular header or implementation file.

@d235j
Copy link
Contributor Author

d235j commented Nov 13, 2015

Feel free to make any changes as needed. Also note that I didn't have a chance to test on Windows.

@russell-taylor
Copy link
Contributor

We may want to go ahead and put a default RenderManager config in place (see the installer for directmode one for the HDK). This won't hurt clients that don't use RenderManager but would make it work by default without a special config file when they are.

@rpavlik
Copy link
Member

rpavlik commented Nov 13, 2015

@russell-taylor we can do that in a separate issue, this one is primarily about cross-platforming stuff.

@d235j

Before we can claim full autoconfig support we also need to implement plugin directory path detection.

Not sure what you mean here - we do already have a plugin directory search path thing. It doesn't handle a system-wide or user-wide thing, but it does correctly handle (I think! I really need to set up a bare-metal Linux machine here) finding + loading plugins from the install tree.

@d235j
Copy link
Contributor Author

d235j commented Nov 13, 2015

@rpavlik you're right, I should have noticed this, since I played with it before.

@d235j
Copy link
Contributor Author

d235j commented Nov 17, 2015

Any thoughts on where we want to go with this? It may need some restructuring, but I'm not sure what exactly I should be doing.

@JeroMiya
Copy link
Contributor

JeroMiya commented Apr 6, 2017

@d235j hope you don't mind, I am in the process of rebasing this PR and addressing @rpavlik's feedback, as well as including some search path generalization code from @godbyk. I may need to push a new branch/PR and close this one, but I wanted to give you a heads up. Thanks for this work!

@d235j
Copy link
Contributor Author

d235j commented Apr 11, 2017

@JeroMiya I don't mind, unfortunately I haven't had much of a chance to look at this stuff lately.

@JeroMiya
Copy link
Contributor

Cool, thanks. I'm going to go ahead and close this PR out then. It's being replaced by this one:
#534

@JeroMiya JeroMiya closed this Apr 11, 2017
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.

4 participants