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

Improve doc #846

Merged
merged 4 commits into from
Jan 12, 2022
Merged

Improve doc #846

merged 4 commits into from
Jan 12, 2022

Conversation

nmellado
Copy link
Contributor

@nmellado nmellado commented Jan 12, 2022

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Docs have been added / updated (for bug fixes / features)

Be aware that the PR request cannot be accepted if it doesn't pass the Continuous Integration tests.

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Update documentation structure and content

  • What is the current behavior? (You can also link to an open issue here)
    Compilation instructions and dependency management is not clear and spread across pages

  • What is the new behavior (if this is a feature change)?
    Simplify documentation to help new users understanding the build chain and use it.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:

@nmellado nmellado changed the base branch from master to release-candidate January 12, 2022 08:23
@nmellado nmellado force-pushed the improve_doc branch 2 times, most recently from e216275 to c8e6e3c Compare January 12, 2022 09:48
@nmellado nmellado marked this pull request as ready for review January 12, 2022 09:50
@nmellado nmellado added documentation Related to documentation enhancement Type of issue/PR: enhancement labels Jan 12, 2022
@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #846 (24029b5) into release-candidate (8eacea0) will not change coverage.
The diff coverage is n/a.

❗ Current head 24029b5 differs from pull request most recent head cd01abe. Consider uploading reports for the commit cd01abe to get more accurate results
Impacted file tree graph

@@                Coverage Diff                 @@
##           release-candidate     #846   +/-   ##
==================================================
  Coverage              26.41%   26.41%           
==================================================
  Files                    321      321           
  Lines                  21410    21410           
==================================================
  Hits                    5656     5656           
  Misses                 15754    15754           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bc3992...cd01abe. Read the comment docs.

Copy link
Contributor

@MathiasPaulin MathiasPaulin left a comment

Choose a reason for hiding this comment

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

Seems quite a good improvement in readability. I'll fetch and compile the doc to verify the new structure and content.
BTW, perhaps you can add somewhere that we encourage users to have dedicated build directory and install directory (the later being manage automatically if the default install dir is used) for each build type (Release, Debug) and that compiling Radium in Debug mode needs to have the dependencies (at least assimp) compiled and installed in Debug mode.

doc/basics/compilation.md Outdated Show resolved Hide resolved
doc/basics/dependencies.md Outdated Show resolved Hide resolved
@nmellado
Copy link
Contributor Author

Seems quite a good improvement in readability. I'll fetch and compile the doc to verify the new structure and content.
BTW, perhaps you can add somewhere that we encourage users to have dedicated build directory and install directory (the later being manage automatically if the default install dir is used) for each build type (Release, Debug) and that compiling Radium in Debug mode needs to have the dependencies (at least assimp) compiled and installed in Debug mode.

Hum right, this is suggested at multiple places, but not clearly stated. I'll let you do you review run, and we will add that with your other suggestions (if any).

doc/CMakeLists.txt Outdated Show resolved Hide resolved
doc/CMakeLists.txt Outdated Show resolved Hide resolved
doc/basics/compilation.md Outdated Show resolved Hide resolved
doc/basics/compilation.md Outdated Show resolved Hide resolved
doc/main.md Outdated Show resolved Hide resolved
doc/basics/dependencies.md Outdated Show resolved Hide resolved
@nmellado nmellado force-pushed the improve_doc branch 2 times, most recently from 24029b5 to 7c2912f Compare January 12, 2022 13:16
@dlyr
Copy link
Contributor

dlyr commented Jan 12, 2022

Maybe clean the history before merge ? (or squash, but I think two commit may be better)

@MathiasPaulin
Copy link
Contributor

In the next days (or weeks), I'll propose a new section on "How to write your own plugin" so that even applications that do not support plugins can used the added functionalities (e.g. headless app). I gave an online course to master students, on the spirit of the "ExamplePluginWithLib" test folder, on how to do that and I just have to clean the used repo before submitting it as a PR.

@nmellado
Copy link
Contributor Author

In the next days (or weeks), I'll propose a new section on "How to write your own plugin" so that even applications that do not support plugins can used the added functionalities (e.g. headless app). I gave an online course to master students, on the spirit of the "ExamplePluginWithLib" test folder, on how to do that and I just have to clean the used repo before submitting it as a PR.

Btw, there is already a page called how to write your own plugin in the developer manual

@nmellado nmellado merged commit dedcd39 into release-candidate Jan 12, 2022
@nmellado nmellado deleted the improve_doc branch January 12, 2022 20:46
@MathiasPaulin
Copy link
Contributor

Btw, there is already a page called how to write your own plugin in the developer manual

Sure, there is one and it is helpful for developing plugins.
But I think that this way of extending Radium by developing plugins is very limited as only BaseApplication derived apps will benefit from the functionalities.
If we want also headless-app (or non Qt-based GUI apps) to benefit from these, we should prefer to develop a library exposing the functionality and interface this library with BaseApplication derived apps by developing a plugin while headless-app will directly link to the library.
This way of extending Radium was the aim of the demo "ExamplePluginWithLib" but, for now, this demo was not updated according to recent PR and does not compile anymore (it is not included in the CI nor in the tests).
There are also other advantages of extending Radium by a lib + plugin, we will discuss about this in a next steering committee.

ntsh-oni pushed a commit to ntsh-oni/Radium-Engine that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation enhancement Type of issue/PR: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants