-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feat: Site deployment #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting this together! Deploying the draft version had been on my mind but I never got around it.
I left a few comments/questions. Also adding @hollasch in case he has any suggestions.
@@ -3896,5 +3896,4 @@ | |||
<link rel='stylesheet' href='../style/book.css'> | |||
<style class="fallback">body{visibility:hidden;white-space:pre;font-family:monospace}</style> | |||
<script src="markdeep.min.js"></script> | |||
<script src="https://morgan3d.github.io/markdeep/latest/markdeep.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this line necessary? The footer of this file is taken verbatim from the Markdeep Get Started page and that includes both script tags. If it doesn't hurt to keep it then I suggest not removing it since it matches those instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we can be sure that we'll include it in the deployment of the site we don't need to have it. It just is more efficient in caching the script for the entire site as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I cannot be sure of this, you'll get a CORS error if you try to include the script tag and check your browser console. Hence, you are either required to add crossorigin="anonymous"
to the script tag or have markdeep.min.js
be included in the project. This might be an oversight by the author and having the script included with the project is probably more than enough and allows us to change it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollasch Do you know if this has been an issue with RTW? It may by worth clarifying with Morgan since you're already in touch with him.
Personally, I agree with @KorigamiK in that this feels redundant since we already host our own copies of markdeep.min.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's explicitly redundant. The idea is to pick up any Markdeep fixes/improvements immediately and automatically. We do update the backup copy in the project, but often with a lot of lag (since I'm not always monitoring Markdeep releases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this has been an issue with RTW?
I wouldn't say it's an problem, but I can't think of a reason not to adopt the recommended protocol, so I've added these throughout in the RTW project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the versioning argument makes sense. In that case I'm inclined to leave this in to stay consistent with the rest of the RTW project.
@@ -12,5 +13,4 @@ | |||
<link rel='stylesheet' href='../style/book.css'> | |||
<style class="fallback">body{visibility:hidden;white-space:pre;font-family:monospace}</style> | |||
<script src="markdeep.min.js"></script> | |||
<script src="https://morgan3d.github.io/markdeep/latest/markdeep.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment around this just following the "Get Started" instructions at https://casual-effects.com/markdeep/#getstarted
Is there an associated issue I can look at that explains the reason for static site deployment? The whole point of Markdeep is that it should not require any static site deployment. An |
By the way, one important goal of the RTW project is that users can create a local clone and start browsing the content immediately and locally without requiring any sort of static site generator infrastructure. |
You're right, and I was introducing more complexity than needed. The latest changes can deploy the site through the Please feel free to change the content of the |
No, they're not necessary. We host the Ray Tracing in One Weekend site and all books via GitHub pages without any deployment process. That's the joy of Markdeep. |
I've removed static site generation. Github pages work fine without deployment now. You can review the changes. |
I'm heading out for now, but will review these later. Or @armansito, feel free to merge and we can iterate further if necessary. |
I'll make the corresponding updates to the RTW |
I have a query out to Morgan asking why the Regarding the Markdeep version we use, we always prefer the latest, but add a reference to the snapshotted library included in the project as a backup in case you're reading the document offline. We should not remove the latest library preference. |
style/book.css
Outdated
@@ -143,13 +149,13 @@ div.indented { | |||
|
|||
/* Code Line Types */ | |||
|
|||
.md code > .highlight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a CSS syntax error, and prefer spaces around the child selector >
operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think this looks better with the spaces, so please revert this part back unless this is strictly necessary.
@@ -171,7 +177,7 @@ div.indented { | |||
.md img { | |||
margin-top: 1.0em; | |||
width: 72ex; | |||
//image-rendering: crips-edges; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "crisp-edges".
Also, @armansito — if you look at the RTW Markdeep source, search for the string "class='pixel'". This image (or figure) class ensures that images are rendered in a pixelated fashion without edge smoothing. We generally use this class when showing render results, but don't use it for things like figures and other drawings. You may want to follow the same approach instead of rendering every image in pixelated fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh neat, I wasn't aware of that. I guess one merit to rendering the images (the renderer outputs) as pixelated is that the per-pixel averages are easier to see that way. All of the figures in the GPU book are SVGs so they already get specially treated by the browser and won't be pixelated. I'll take a look at this later and align the style with RTW.
- Inspired by RayTracing/gpu-tracing#9, I'm adopting the CSS code style of having one selector per line with trailing commas for definitions that apply to multiple selectors. - Corrected "Hilight.js" to "Highlight.js". There _is_ a hilight.js, but it looks like the syntax highlighting package used by Markdeep is highlight.js (https://highlightjs.org/). - Fixed background-color syntax bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KorigamiK Thanks for the update. I think this looks good. There are a couple remaining comments by @hollasch regarding CSS style. I'll approve the PR once you address those.
Once these changes are in, I'll go ahead and set up the static site deployment.
style/book.css
Outdated
@@ -143,13 +149,13 @@ div.indented { | |||
|
|||
/* Code Line Types */ | |||
|
|||
.md code > .highlight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think this looks better with the spaces, so please revert this part back unless this is strictly necessary.
@@ -3896,5 +3896,4 @@ | |||
<link rel='stylesheet' href='../style/book.css'> | |||
<style class="fallback">body{visibility:hidden;white-space:pre;font-family:monospace}</style> | |||
<script src="markdeep.min.js"></script> | |||
<script src="https://morgan3d.github.io/markdeep/latest/markdeep.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollasch Do you know if this has been an issue with RTW? It may by worth clarifying with Morgan since you're already in touch with him.
Personally, I agree with @KorigamiK in that this feels redundant since we already host our own copies of markdeep.min.js
.
@armansito sounds good, the static site should work on enabling gh-pages on the |
- Inspired by RayTracing/gpu-tracing#9, I'm adopting the CSS code style of having one selector per line with trailing commas for definitions that apply to multiple selectors. - Corrected "Hilight.js" to "Highlight.js". There _is_ a hilight.js, but it looks like the syntax highlighting package used by Markdeep is highlight.js (https://highlightjs.org/). - Fixed background-color syntax bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please squash and merge when you're ready.
@@ -3897,4 +3897,5 @@ | |||
<link rel='stylesheet' href='../style/book.css'> | |||
<style class="fallback">body{visibility:hidden;white-space:pre;font-family:monospace}</style> | |||
<script src="markdeep.min.js"></script> | |||
<script crossorigin="anonymous" src="https://morgan3d.github.io/markdeep/latest/markdeep.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added script tags to the latest markdeep library.
It should be noted that without the crossorigin="anonymous"
attribute the script would not load for anyone
due to CORS.
This is also an issue of the markdeep starter templates, which should be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the local script line and the crossorigin attribute, and you'll find that the script loads just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you don't start a local dev server when you test this?
When you open an HTML file using the file:/// protocol, CORS restrictions are typically not enforced by browsers.
Refer: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#file_origins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://hollasch.net/rtw/books/RayTracingInOneWeekend.html. This version has no local script present, nor referenced, pulling only from the latest Markdeep version. Page loads and renders without issue (Brave browser).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More information on the crossorigin
attribute: https://stackoverflow.com/questions/41069330/with-script-crossorigin-anonymous-why-is-a-script-blocked-by-cors-policy
It looks like the only reason to add crossorigin='anonymous'
is "if we care about getting error information for the script being loaded", which we don't. Default behavior is specified by explicitly omitting the crossorigin
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but for some reason I get NS_ERROR_FAILURE
on firefox when I first load the html page on localhost. it opens after i load it again. not sure what's the reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definitely sounds annoying. We treat the HTML docs as standalone documents, so don't normally fire up a web host just to read them. This is how we expect our readers to view the local documents as well.
@hollasch if you are already in conversation with Markdeep, you should also request to add |
The site is now deployed at https://raytracing.github.io/gpu-tracing. |
Really appreciate the work you put into the book.
But as of now users are not able to see the draft deployed, so I have created a workflow with some fixes to deploy the site to github-pages.
Please look at the commit messages for the changes.
Once merged the site would look something like this: https://korigamik.is-a.dev/gpu-tracing/
but on the main repo which would be https://raytracing.github.io/gpu-tracing.
Cheers!