Open svnkomo opened 7 months ago
@svnkomo Hello Sakhile! This is fantastic! I will definitely be merging this, as I think that this is a perfect addition to the path tracing repo here. I happened to check my email late tonight right before bed (ha) which had the notification about your PR. If you don't mind, let me get some sleep and go over everything in detail tomorrow (Friday). But from what I've seen and tested out so far, this is amazing!
Thank you so much, and talk to you soon! :-) -Erich
@erichlof Hi Erich! No problem, it's something I've been thinking of doing for a while, finally got down to it. Glad you like it. If there are any suggestions or improvements to be made, I'd be glad to do it. Definitely a WIP.
Hi again, Sakhile! For the most part everything looks great - there are just a couple of issues I have found and a couple of questions/requests that I want to ask.
I think all of the demos are listed on the examples page - the only one I couldn't find is Geometry_Showcase.html (that's the main default demo for my repo - it has a link at the top right when you visit the GitHub repo). Could you please add a .png card for that as well, maybe like you did for Quadric_Geometry_Showcase.html, which is very similar?
The only other issue I found was that the buttons don't appear correctly anymore when viewing on my mobile device. I'm including a screenshot:
The buttons on the right appear shifted down and off the viewing area, so that mobile users can't navigate the camera. The good news is that I'm sure this can be fixed with a 'one-shot-fixes-all' solution (maybe in the default CSS?), but I'm not very knowledgeable about CSS and not very up-to-date with how tailwind works. Those triangular buttons for mobile might have to be 'fixed' in their position. Long ago I cobbled together the correct spacing for both portrait and landscape mode for mobile, so that no matter which way you turn your phone or tablet, the buttons are visible and accessible in the corner. Here's the source code found inside InitCommon.js: https://github.com/erichlof/THREE.js-PathTracing-Renderer/blob/4f6c03b75afb9799e522ad98302d633ea2c52ae5/js/InitCommon.js#L190-L231
Each time the window is updated (called at app startup and whenever a user rotates the mobile device or resizes the window), the above code correctly spaces the mobile triangular buttons. There's a small room for error when dealing with mobile buttons like these, because you don't want them too spread out where they won't all fit on the page, but then if they're too close together, some people's thicker thumbs accidentally press 2 buttons at once, which is frustrating. I would like to keep this spacing in place, but if you have a better, more modern css solution that reflects these spacings, I'm open to new suggestions! I'm sure you and I can figure this issue out.
This reply is getting long so I'll put the questions in the next one... Thank you! 😊
Hi again, I wanted to ask some questions about the new links and files. Is it possible to just include the tailwind.js file on the repo here, instead of dynamically linking to a cdn? As you know, I like to have everything available in one place on the repo here, even three.js, so that when someone forks or downloads the source code, they can quickly just click the demos on their own local machine (with a minimal local server running of course, for images and shader file loading) and it'll just work. If they happen to be not connected to the internet, I would like them still to be able to start a local simple server and run everything right off the bat. The cdn link to tailwind wouldn't work in that situation, unless I'm mistaken. I know that adds a little size to the repo, but I would just like all dependencies in one place, so that if a user is offline, or tailwind goes away or has breaking changes, then this tailwind file we're including right now is a 'frozen' version that correctly works out of the box. I looked at the cdn link and it does appear already minified, so this current version and file size is good to go in my eyes. Again, open to suggestions on how to handle this dependency. btw, the only reason I go through the effort of manually updating the three.module.min.js file every month (when they have a new version release), is that sometimes there are breaking changes, but three.js is constantly being worked on and improved - so I want to deal with the latest, optimized version as we go through time. But that's just because it is at the core and framework of everything here and is the 'glue' that holds everything together and allows all the demos to function properly. So I don't think we need to worry about the tailwind file in this 'mission-critical' way - if the current minified version works and will work without fuss in the future, then I'm ok with that. :)
Lastly, will everything be hosted from my repo here? I noticed that the examples page is currently hosted by your GitHub pages, but unless I'm mistaken, that's just so you can show the functionality while you're working on your fork of my repo - do I have this correct in my mind?
Again, thank you so much for putting all of this together- I know it must have been a lot of work! Once we get some of these issues and details worked out, your new examples system will take this GitHub repo to the next level! 😃
Talk to you soon, Erich
Proposal to add an abridged version of all current and future path tracing examples.