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

imagebuf docs examples: c++ examples, outfile, run script, and doc (#3992) #4020

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

Conversation

MichaelRCarroll-Intel
Copy link

Description

This PR address the docs files with accompanying examples #3992.
The initial commit has c++ only to check if the PR is on the right track. Python can be added next.

  • Unfortunately, I still can't get the ctest test bed to run automagically. So I'm blocked on checking the full suite of SHAs and text output via ref/out.txt.
  • I was able to run the docs-examples-imagebuf.cpp as a one off and get sensible results.
  • The original inline example source needed minor fixes to a couple of breaks. It would be time consuming for newbies to resolve, so the example source changes are sorely needed.

As of time of submission my company is still getting the CLA together. Expectation is this PR will help for review but be blocked from merge until CLA is resolved. Reference the #devdays slack channel 11:47a california time 20231013 Sean McDuffe thread.

Sidebar: A linux foundation appropriately put a contribution on this PR into pending. This PR is from a colleague at my company.

Thanks.

Tests

Testing was performed on Ubuntu 22.04 w/ GNU 11.4 system compiler.
Result image info and SHAs were generated with oiiotool from a build from yesterday's master branch.

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • If I added or modified a C++ API call, I have also amended the
    corresponding Python bindings (and if altering ImageBufAlgo functions, also
    exposed the new functionality as oiiotool options).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format before submitting, I definitely will look at the CI
    test that runs clang-format and fix anything that it highlights as being
    nonconforming.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 13, 2023

CLA Not Signed

@MichaelRCarroll-Intel
Copy link
Author

Forgot to sign with -s, so I amended previous commit and force pushed.

@lgritz
Copy link
Collaborator

lgritz commented Oct 14, 2023

I believe that the CI failures are all because docs-examples-cpp/ref/out.txt needs to be updated.

Comment on lines +377 to +378
:start-after: BEGIN-imagebuf-get-pixel-average
:end-before: END-imagebuf-get-pixel-average
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the C++ code, you called this BEGIN/END-imagebuf-get-pixel-avg

:language: c++
:start-after: BEGIN-imagebuf-set-region-black
:end-before: END-imagebuf-set-region-black
:dedent: 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's important not to us dedent here, because the code you're cutting from already starts in column 0.

}
return true;
}
.. tabs::
Copy link
Collaborator

@lgritz lgritz Oct 14, 2023

Choose a reason for hiding this comment

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

This needs to be

Suggested change
.. tabs::
.. tabs:

In Sphinx rst, a :: ending a text sentence means "the next indented region is code". So if we replace the ":: followed by indented region" with a .. tab:: section, we don't want that treated as a code example, we want it interpreted as commands.

@@ -474,37 +445,14 @@ Strategy 2: Template your iterating functions based on buffer type
Consider the following alternate version of the `make_black` function
from Section `Example: Set all pixels in a region to black`_ ::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another spot where the :: needs to change to : because we replaced intended code itself with .. tabs:: commands.

@@ -515,22 +463,14 @@ In fact, :file:`imagebufalgo_util.h` provides a macro to do this (and
several variants, which will be discussed in more detail in the next
chapter). You could rewrite the example even more simply::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
chapter). You could rewrite the example even more simply::
chapter). You could rewrite the example even more simply:

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

The bones of this are great. I made some comments to point out fixes to problems I saw when I built the docs on my end: some :: -> :, and one place where the begin/end clipping strings didn't match between the code and the docs referencing the code.

Fix those up, and the reference output of that test and we should be good to go.

@grdanny
Copy link
Contributor

grdanny commented Oct 15, 2023

I'm going to tackle the make texture example in the imagebufalgo doc.

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.

3 participants