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

[imgui] Add experimental docking feature #13899

Merged
merged 10 commits into from Dec 15, 2020
Merged

[imgui] Add experimental docking feature #13899

merged 10 commits into from Dec 15, 2020

Conversation

brukted
Copy link
Contributor

@brukted brukted commented Oct 6, 2020

One thing worth noting is that the docking branch is still in beta. But as expressed in the readme of the repo it is pretty much stable and used by many software.

@LilyWangL LilyWangL changed the title add docking support for imgui [imgui] add docking support for imgui Oct 9, 2020
@LilyWangL LilyWangL added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 9, 2020
@RT2Code
Copy link
Contributor

RT2Code commented Oct 9, 2020

DESTINATION include/${PROJECT_NAME}

Is moving the headers to the imgui directory really necessary? This would break every port depending of it.

@brukted
Copy link
Contributor Author

brukted commented Oct 9, 2020

@RT222 It's necessary since both the master and docking branch files have same names and installing imgui-docking would overwrite the existing files. But It shouldn't break other ports because the include directories will be changed accordingly.

target_include_directories(
	${PROJECT_NAME}
	PUBLIC
		$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
		$<INSTALL_INTERFACE:include>/${PROJECT_NAME}
)

@LilyWangL LilyWangL changed the title [imgui] add docking support for imgui [imgui-docking] Add new port Oct 10, 2020
@LilyWangL LilyWangL added category:new-port The issue is requesting a new library to be added; consider making a PR! and removed category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Oct 10, 2020
@LilyWangL LilyWangL added info:reviewed Pull Request changes follow basic guidelines requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function and removed info:reviewed Pull Request changes follow basic guidelines requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Oct 10, 2020
@dan-shaw dan-shaw added requires:discussion and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 13, 2020
@BillyONeal
Copy link
Member

@dan-shaw Why did you tag this requires:discussion?

@LilyWangL
Copy link
Contributor

Pinging @dan-shaw for response.

@BillyONeal BillyONeal changed the title [imgui-docking] Add new port [imgui-docking] Add new port as an alternative of imgui Nov 2, 2020
@BillyONeal
Copy link
Member

@dan-shaw confirmed with me out of band that we are unsure whether we want to catalog what are effectively different versions of the same library under different names. He says that he, @strega-nil, and @ras0219 / @ras0219-msft discussed it a few weeks ago but didn't end up at a consensus. I'll keep you posted.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

@ocornut @rokups @ShironekoBen This is really about how you would like imgui's "docking" feature branch exposed in vcpkg. Does it make sense to have it available as an incompatible alternative like the boringssl vs. openssl split, as proposed here? Are you OK with marking whichever merge commit merges master at a given imgui version into the docking branch as "imgui %VERSION%"? Should we use some external configuration (via a user-customized triplet) that indicates desire to use the docking branch?

ports/imgui-docking/CONTROL Outdated Show resolved Hide resolved
@strega-nil
Copy link
Contributor

Then what I think is probably that this is a use for versioning (and specifically, beta versions).

@BillyONeal BillyONeal added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response labels Nov 8, 2020
@BillyONeal
Copy link
Member

@rokups Thanks so much for your feedback.

@brukted Given the wishes / indications of the upstream library maintainers I think we want to wait to accept this until the versioning story @vicroms is working on lands and hook it up to that.

@ocornut
Copy link

ocornut commented Nov 8, 2020

I am not sure if merges to docking are done during releases, so there might not be a precise v1.78 docking version for example.

Just to clarify, merges into docking are done very regularly (1-2 months a month at least) and definitively right after every time a release is tagged for master.

Difficult for me to say how it should be catalogued in vcpkg, out of context I would say a YYYYMMDD versioning scheme + short git hash would neatly convey the kinda wip-experimental aspect of this branch, but "1.79+docking" would also work. I understand there's lot of demand for this branch and many users have switched to it a long time ago, and therefore it makes sense to feature it in vcpkg.

@BillyONeal
Copy link
Member

@ocornut Can we expect APIs that are in master to work more or less identically in the "docking" branch which are not specifically related to the docking feature?

We are considering recommending using "features" to model this: a "docking" feature which would select the corresponding merge commit from master to docking for a given release in master.

The north star for "features" is that turning on a feature should not break people who don't care about the presence of the feature. Do you believe that there is a high probability that people who get docking when they didn't explicitly ask for it would be OK, or are they likely to be broken? Examples that would make them likely to be broken would be API removals. For example, we couldn't model boringssl vs openssl with features but we might be able to model this.

Thanks for your help!

@BillyONeal
Copy link
Member

To clarify: if the probability of breakage is high, we must do what this PR does and add a skip in ci.baseline.txt; otherwise, we need to change this to have a docking feature.

@ocornut
Copy link

ocornut commented Dec 10, 2020 via email

@BillyONeal
Copy link
Member

@brukted Can you change this such that the docking branch commit corresponding to the current master version commit is selected if and only if a feature docking-experimental is enabled in the plain imgui port?

Thanks!

@BillyONeal BillyONeal added requires:author-response and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Dec 10, 2020
@brukted
Copy link
Contributor Author

brukted commented Dec 13, 2020

@BillyONeal Ok.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please bump the port version. See documentation.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 15, 2020
@JackBoosY
Copy link
Contributor

Should be good, @BillyONeal do you have any other questions?

@BillyONeal BillyONeal changed the title [imgui-docking] Add new port as an alternative of imgui [imgui] Add experimental docking feature Dec 15, 2020
@BillyONeal
Copy link
Member

I verified that 682249396f02b8c21e5ff333ab4a1969c89387ad is the right merge commit and @JackBoosY's request for a bumped port version is satisfied.

Thanks for your contribution and sorry for the deliberations!

@BillyONeal BillyONeal merged commit 7e50ef0 into microsoft:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants