Skip to content

Conversation

grahamreeds
Copy link

Note: I developed this in a container as not to pollute my wider WSL installation.

This may not work as intended as I am currently having trouble with the project that intends to consume this having trouble building and having it exist in its workspace (another docker container).

@giladreich
Copy link
Owner

Hi @grahamreeds, thanks for the PR! :) I'll try to have a look sometime this week or weekend and hopefully merge it if all good.

Copy link
Owner

@giladreich giladreich left a comment

Choose a reason for hiding this comment

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

Hi @grahamreeds, I just briefly had a look at your changes.

First off, many thanks for your efforts! It looks great for the most part.

Secondly, I'm afraid there is a problem with my initial approach of how I implemented the dependent cmake options (cmake_dependent_option). This will lead to other problems as we add more platforms and renderers (backends) in the long run.

I believe since we're externalizing this library and there could be so many combinations between the variant of Platforms and Renderers (e.g. SDL2 platform could run with dx{9,10,11,12}, vulkan, opengl{2,3} or sdlrenderer{2,3} etc.. renderers), then the existing approach won't scale. Therefore we need to completely separate the examples from the main build. This means if I know now I'm going to use ImGui with SDL2 and DX11, then the scope of the build should only be for what I chose.

I also remember I added myself a note here:

# TODO: Validate configurations based on the given input combination

to add validation for the given options, since I probably realized at the time that this doesn't scale.

Let me think about it more. Maybe tomorrow in the evening I may have some time to look or weekend and update this repo a little and add more examples.

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.

2 participants