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

Initial cleanup for 1.0. #191

Merged

Conversation

portsmouth
Copy link
Contributor

Add direct link to parameter reference from README.

Add direct link to parameter reference from README.
@portsmouth portsmouth changed the title Improve text on GitHub README. Final cleanup for 1.0. May 14, 2024
@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

I strongly suggest we make these links not visible from the GitHub front page, as we don't want people navigating to those files directly, as then the markdown does not render. (Some people already mistook the un-rendered markdown source as the intended form of the spec).

That can be done by putting them in a sub-folder, named e.g. internal.

Someone with admin access is probably needed to do that, as it may require pointing the GitHub Pages link to the internal/index.html etc. See the dialog below for that.

(A better solution is to make a nicer landing page for OpenPBR, that is not the GitHub repo itself. But that would take some time to create).

image

image

Specification -> White paper
@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

Clarified front page:

image

@portsmouth portsmouth marked this pull request as ready for review May 14, 2024 02:04
@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

The spec takes about 10 seconds to properly render (due to all the MathJax I assume), maybe we should mention that next to the white paper link?

@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

Added a copy-pastable BibTeX citation reference, which is needed if we want OpenPBR to be referenced in papers or documentation. (The form of this can be tweaked as needed, we can check how it renders in a paper for example).

@techreport{OpenPBR,
  author = {Zap Andersson and Paul Edmondson and Julien Guertault and Adrien Herubel and Alan King and Peter Kutz and Andréa Machizaud and Jamie Portsmouth and Frédéric Servant},
  title = {OpenPBR Surface},
  institution = {Academy Software Foundation (ASWF)},
  year = {2024},
  url = {https://academysoftwarefoundation.github.io/OpenPBR/}
}

@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

Added a clickable GitHub icon to the spec (just after the version info), which links back to the GitHub page.

Also removed "specification" from the title. I think that is reasonably implied (and we say it in the first sentence of the abstract), and makes a more bold and readable title.

image

@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

The reference to the Gulbrandsen paper is missing, even though we cite it.

Added in 0c69b27.

@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

Rewording the text on the front page in ac84bfe, as the sentence was long and hard to parse.

OpenPBR Surface is a specification of a surface shading model intended as a standard for computer graphics. It aims to provide a material representation capable of accurately modeling the vast majority of CG materials used in practical visual effects and feature animation productions.

OpenPBR Surface is an open standard hosted by the Academy Software Foundation (ASWF), and is organized as a subproject of MaterialX.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
citation.bib Outdated Show resolved Hide resolved
@virtualzavie
Copy link
Contributor

Someone with admin access is probably needed to do that, as it may require pointing the GitHub Pages link to the internal/index.html etc. See the dialog below for that.

Internal doesn't seem like a faithful description of the nature of those files. Accessing them directly is only problematic on GitHub, but works fine for someone who clones the repository.
What about specification/index.html or spec/index.html? This way they are moved away from top-level, but are not "hidden".

@virtualzavie
Copy link
Contributor

The spec takes about 10 seconds to properly render (due to all the MathJax I assume), maybe we should mention that next to the white paper link?

On my side, it loads nearly instantaneously, both on Firefox/macOS, Firefox/Windows and Edge/Windows.

@portsmouth
Copy link
Contributor Author

The spec takes about 10 seconds to properly render (due to all the MathJax I assume), maybe we should mention that next to the white paper link?

On my side, it loads nearly instantaneously, both on Firefox/macOS, Firefox/Windows and Edge/Windows.

Do the equations render correctly nearly instantaneously? On my system, they show up immediately, but change format as they are correctly rendered.

@virtualzavie
Copy link
Contributor

[page loading time]

Do the equations render correctly nearly instantaneously? On my system, they show up immediately, but change format as they are correctly rendered.

After maybe a second or so, yes.
The loading of images is what takes the most time on my side, with the scrolling being a bit all over the place while this happens.

openpbr.bib Outdated
@@ -3,5 +3,7 @@ @techreport{OpenPBR
title = {OpenPBR Surface},
institution = {Academy Software Foundation (ASWF)},
year = {2024},
month = {May},
version = {1.0},
Copy link
Contributor

Choose a reason for hiding this comment

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

A note of caution to @jstone-lucasfilm for the release candidate, that we now have a third place where the version number appears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced a version number is needed in a citation. It can be assumed that the reference will want to refer to the latest version, which is available at the given link.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that the version number is not usually included in citations, so this may not end up being an issue in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it for now.

@portsmouth
Copy link
Contributor Author

Someone with admin access is probably needed to do that, as it may require pointing the GitHub Pages link to the internal/index.html etc. See the dialog below for that.

Internal doesn't seem like a faithful description of the nature of those files. Accessing them directly is only problematic on GitHub, but works fine for someone who clones the repository. What about specification/index.html or spec/index.html? This way they are moved away from top-level, but are not "hidden".

Sounds fine to me. But I can't make this change myself as it will break the GitHub pages deployment. @jstone-lucasfilm would you mind to make this change?

@jstone-lucasfilm
Copy link
Member

@portsmouth For moving the specification to a new location, I'm happy to assist, but I'd recommend that we leave this as an improvement for after today's release candidate.

@portsmouth
Copy link
Contributor Author

@portsmouth For moving the specification to a new location, I'm happy to assist, but I'd recommend that we leave this as an improvement for after today's release candidate.

Sure that's fine. How about we merge the other improvements in this PR, and leave that for a different PR?

@jstone-lucasfilm
Copy link
Member

@portsmouth Sure, I'm fine for this PR to be the first in a series of improvements over the next two weeks, but then maybe the title of the PR should be more specific? :D

@portsmouth portsmouth changed the title Final cleanup for 1.0. Initial cleanup for 1.0. May 14, 2024
@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

@portsmouth Sure, I'm fine for this PR to be the first in a series of improvements over the next two weeks, but then maybe the title of the PR should be more specific? :D

OK, changed the PR title.

How is this? The ASWF logo now doubles as the link back to the OpenPBR GitHub, killing two birds with one stone. (If you prefer another form of that logo, or a different size/placement, feel free to suggest).

image

I moved the authors to the end, and took the liberty of moving @jstone-lucasfilm to that list, as that seems fairer to me considering your contribution. The BibTeX citation is updated accordingly, with the version removed.

image

@techreport{OpenPBR,
  author = {Zap Andersson and Paul Edmondson and Julien Guertault and Adrien Herubel and Alan King and Peter Kutz and Andréa Machizaud and Jamie Portsmouth and Frédéric Servant and Jonathan Stone},
  title = {OpenPBR Surface},
  institution = {Academy Software Foundation (ASWF)},
  year = {2024},
  month = {May},
  url = {https://academysoftwarefoundation.github.io/OpenPBR/}
}

@portsmouth
Copy link
Contributor Author

Let me add the shader playground image as well...

@virtualzavie
Copy link
Contributor

I think we can thank a few more people who have made significant contributions in the last months.
Chris Kulla, Eugene d'Eon, Masuo Suzuki, Andrea Weidlich, André Mazzone and Stephen Hill come to mind.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@portsmouth
Copy link
Contributor Author

portsmouth commented May 14, 2024

How's this? (Nikie is attributed in the alt-text of the images).

image

portsmouth and others added 2 commits May 14, 2024 22:45
Co-authored-by: Julien Guertault <9511025+virtualzavie@users.noreply.github.com>
Signed-off-by: Jamie Portsmouth <jamports@mac.com>
Co-authored-by: Julien Guertault <9511025+virtualzavie@users.noreply.github.com>
Signed-off-by: Jamie Portsmouth <jamports@mac.com>
@portsmouth
Copy link
Contributor Author

I think we can thank a few more people who have made significant contributions in the last months. Chris Kulla, Eugene d'Eon, Masuo Suzuki, Andrea Weidlich, André Mazzone and Stephen Hill come to mind.

Good point, thanks for the list. I updated it to:

image

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @portsmouth.

@jstone-lucasfilm jstone-lucasfilm merged commit 7d7c2e4 into AcademySoftwareFoundation:main May 14, 2024
1 check passed
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