search

X-Ryl669 / grav-plugin-viewer3-d

A 3D Viewer plugin for Grav CMS
GNU Affero General Public License v3.0
0 stars 0 forks source link

Review of plugin #1

Closed pamtbaau closed 3 years ago

pamtbaau commented 3 years ago

@X-Ryl669, I've taken the liberty to test-run your plugin. In short: I cannot get it to work...

I proceeded as follows:

Other remarks:

X-Ryl669 commented 3 years ago

So, do you have a javascript error in your browser's console ?

As I said in the other issue, the node <div class="online..." is written with 2 children node at once. From there, you should also have 3 javascript files added to your page (three.min.js, o3dv.min.js and 3dviewer.js). If they aren't loaded or if they fails, it should appear in the Javascript console or the network tab.

X-Ryl669 commented 3 years ago

(Sorry for misreading your description)

X-Ryl669 commented 3 years ago

Do you have jquery on your website ? The plugin requires jquery, but since it's loaded by default in my Grav installation, I haven't included it in the required resources. I suspect it would fail to load if you don't have it in your default installation.

Said differently, does it work when you go here ?

pamtbaau commented 3 years ago

The viewer isn't created because assets are never being loaded: onTwigSiteVariables is fired before onMarkdownInitialized. See https://learn.getgrav.org/17/plugins/grav-lifecycle

X-Ryl669 commented 3 years ago

That's not what written here and it works on my site. In the referred page, onMarkdownInitialized is 19.2th and onTwigSiteVariables is 25th. In your referred page, onTwigSiteVariables (do not confuse with onTwigInitialized) seems to happen after running the page processor. The page processor must render markdown (althrough there is no description as to when page.content() is called), I suppose it's done after onTwigInitialized but before onTwigSiteVariables

X-Ryl669 commented 3 years ago

You can put a var_dump("markdown") in onMarkdownInitialized and var_dump("twig") in onTwigSiteVariables to check in which order they are called. On my website, I'm getting:

string(8) "markdown"
string(16) "twigsitevariable"

So it's clearly behaving like expected for me.

pamtbaau commented 3 years ago

Yes, I believe it is working in your env...

Would you mind taking a step back and pretend to be a new user:

If I perform above steps:

X-Ryl669 commented 3 years ago

That's strange. I've started a website like you said and it behaves differently as you said. I don't understand why, but clearly the documentation is wrong here. In the documentation page here it says that onMarkdownInitialized happens in onPageInitialized (unconditionally off the cached state of the page) before onTwigSiteVariables. This is also what's stated in your linked documentation page: onPageInitialized happens in the page processor (position 10.3), while onTwigSiteVariables should happen later in position 12.3.1.

I think it's a bug with grav or its documentation. Else, no one can load additional assets required by markdown extensions conditionally (short code or markdown, I don't think it would make a change here).

Obviously I could force loading the assets anywhere (replace the line that read if (isset($this->used) && $this->used) by if (true) and it'll work, but that's poor solution since it forces loading threejs library that's huge for all the website's pages.

So, I guess I'll have to fill a bug in Grav's repository here or maybe you know why it's not following the documented lifecycle ?

X-Ryl669 commented 3 years ago

BTW, it's also requiring to add a media type for STL files in your site, typically like this (in Configuration / Media tab): image

pamtbaau commented 3 years ago

Glad you've done the test... I wanted you see for your self which 3 issues (js assets, images, media) were blocking the display of the viewer.

Yes, page Event hooks is misleading and I'm currently in the process of rewriting it. Grav lifecycle is correct and more clear.

You may be misconceiving the aspect of caching used by Grav. Read the bottom of Grav lifecycle

Meaning:

Besides the loading of javascript, two other roadblocks were preventing the viewer to be shown:

Final suggestion: Have a look at the Shortcode Core plugin. That is the Grav way of doing it. When using Shortcodes, the cache_enable trick is no longer needed and the page's content can be efficiently cached. Also, assets are being cached. Quite more efficient...

X-Ryl669 commented 3 years ago

Thanks for your hints, it's much appreciated!

I think changing the media.yaml is the only way that would work, since the plugin allows to download the model and as such, the correct mime-type must be set for downloading it. So if I link to the file directly, I think (not tested), that the http server will not send the correct mime-type by itself (I don't think nginx or apache have the mime-type for all supported 3d model file).

Can I change it automatically in my plugin (instead of asking the user to do it manually) ?

How can I specify in my plugin that it depends on the Shortcode Core plugin ?

pamtbaau commented 3 years ago

How can I specify in my plugin that it depends on the Shortcode Core plugin ?

Inside blueprint.yaml, add something like the following to the dependencies section:

- { name: shortcode-core, version: '>=5.0.0' }

So if I link to the file directly, I think (not tested)

Test it and you'll know it. "The proof is in the pudding..." (not the mind)

X-Ryl669 commented 3 years ago

Test it and you'll know it

Tested and it does not work. Got Content-Type: application/vnd.ms-pki.stl and not model/stl as expected. I don't know where the former is coming from (I don't manage the http server on my test server), but it's clearly incorrect.

X-Ryl669 commented 3 years ago

I'm closing this now, since I'll remove this repository in favor of shortcode's version here