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

Added recipe for Inspector Plugin #48

Merged
merged 24 commits into from
Jun 19, 2024

Conversation

snatvb
Copy link
Contributor

@snatvb snatvb commented Jun 9, 2024

Implementation for docs of official Godot Docs on gdnext (godot rust)

advanced GUI you want to implement.

```admonish hint title="Gameplay-only code"
Use an [`is_editor_hint` guard][api-engine-iseditorhint] if you don't want some code executing during runtime of the game.
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this relevant now? my understanding is that #[class(tool)] takes care of this for you, but I could be wrong

Copy link
Contributor Author

@snatvb snatvb Jun 9, 2024

Choose a reason for hiding this comment

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

I don't know actually, I just moved this file - added only close for the codeblock
@Bromeon could you help with it?

I didn't do it and didn't get any extra calls

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is no longer needed. Would be good to remove!

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
Comment on lines 15 to 17
Plugins written in GDScript are automatically disabled if they have a code error, but because Rust is a compiled language,
you cannot introduce compile-time errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a better way to say this is that errors that that would cause Gdscript-based plugins to be disabled are caught at compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe such:

Plugins written in GDScript are disabled if an error occurs during compilation. Since Rust is a compiled language, your plugin will be registered with the engine immediately, and you won't see it in the list of plugins. An error at runtime or connection can lead to editor crash.

Copy link
Member

Choose a reason for hiding this comment

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

your plugin will be registered with the engine immediately

Immediately when? On load/reload of the dynamic library?

, and you won't see it in the list of plugins.

I think this misses the connection to the error, e.g. "won't see it ... if an error occurs".

An error at runtime or connection can lead to editor crash.

What do you mean with "connection" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, you can change it later as you wish, this is not mine text :)

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
@Bromeon
Copy link
Member

Bromeon commented Jun 9, 2024

Thanks, also to @fpdotmonkey for the review!

Regarding oxipng, the intention was that it would commit the updated PNG files directly, but it seems there's a permission problem since your branch is from another repo. Would need to see how to configure this...

@Bromeon Bromeon added the new-topic New content added to the book label Jun 9, 2024
@snatvb snatvb requested a review from fpdotmonkey June 9, 2024 19:52
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot 😊

There are some more occurrences of the to_variant().to() pattern, which is never needed. Check the parameter type and try to convert directly; I've made some examples.

Comment on lines 43 to 53
```rust
use godot::{
engine::{
Button, EditorInspectorPlugin, EditorPlugin, EditorProperty, IEditorInspectorPlugin,
IEditorPlugin, IEditorProperty,
},
global,
prelude::*,
};
use rand::Rng;
```
Copy link
Member

Choose a reason for hiding this comment

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

Please use flat imports. Also, godot::engine has been renamed to godot::classes.

Copy link
Member

Choose a reason for hiding this comment

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

This is not addressed, please make sure only conversations are resolved that are actually fixed. It's a lot of effort if I have to double-check 20 parallel conversations.

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
Comment on lines +33 to +34
- [Editor plugins](recipes/editor-plugin/index.md)
- [Inspector plugins](recipes/editor-plugin/inspector-plugins.md)
Copy link
Member

Choose a reason for hiding this comment

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

I would probably keep those in one page, as they are closely related and the current page is very short. Just use a h2 for the inspector plugins.

Then, you also wouldn't need a new folder. The images could be named inspector-before/after.png.

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 thought it could be extended with others exmaples 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah you mean others from this list? Good point, then maybe leave it.

Can you add a HTML comment (which isn't rendered) on the main editor plugin site, like

<!-- TODO: more plugins from https://docs.godotengine.org/en/stable/tutorials/plugins/editor/index.html -->

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, have a look

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
@snatvb snatvb requested review from Bromeon and lilizoey June 10, 2024 20:23
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update! Just a small request, please don't mark comments as resolved if you haven't fixed them.

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
Comment on lines 18 to 26
The [example](https://docs.godotengine.org/en/stable/tutorials/plugins/editor/inspector_plugins.html) in the Godot docs in Rust.

Before:

![Before](./images/before.png)

After:

![After](./images/after.png)
Copy link
Member

Choose a reason for hiding this comment

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

If you add a newline, it's still part of the same paragraph. The sentence just continues.

If you add an empty line in between (like <-- here), then it's rendered as new paragraph.

Not exactly sure with images, but the text and image should definitely not be on the same line.

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
Comment on lines 43 to 53
```rust
use godot::{
engine::{
Button, EditorInspectorPlugin, EditorPlugin, EditorProperty, IEditorInspectorPlugin,
IEditorPlugin, IEditorProperty,
},
global,
prelude::*,
};
use rand::Rng;
```
Copy link
Member

Choose a reason for hiding this comment

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

This is not addressed, please make sure only conversations are resolved that are actually fixed. It's a lot of effort if I have to double-check 20 parallel conversations.

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
@snatvb
Copy link
Contributor Author

snatvb commented Jun 10, 2024

Thanks a lot for the update! Just a small request, please don't mark comments as resolved if you haven't fixed them.

I probably misunderstood you on this matter

@Bromeon
Copy link
Member

Bromeon commented Jun 11, 2024

I pushed a commit to reduce the PNG size, CI is now green 🌳

Feel free to squash the commits later and remove my authorship.

@snatvb
Copy link
Contributor Author

snatvb commented Jun 11, 2024

I can't merge because I don't have write access :)
If it's ok to merge - I am ready)
image

@Bromeon
Copy link
Member

Bromeon commented Jun 11, 2024

I meant squash (combine all commits to one), not merge 😉 But it's fine, I should be able to do so upon merge (or manually).

I'll give it another review round in the coming days. Meanwhile, maybe @fpdotmonkey or @lilizoey have something to add 🙂

@snatvb
Copy link
Contributor Author

snatvb commented Jun 11, 2024

@Bromeon maybe I don't know something, but to do squash I have to do force push. It could be used as a merge strategy(where is merge button). 😊

@Bromeon
Copy link
Member

Bromeon commented Jun 11, 2024

Yes, I can do it upon merge. But force-pushing is also no problem, contributors regularly do it to clean up.

It's really up to you. For the main repo, we'd need some level of squashing/force-pushing as I can't do it on merge there 👌

Copy link
Contributor

@fpdotmonkey fpdotmonkey left a comment

Choose a reason for hiding this comment

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

You know, I was wondering why you weren't responding to my review comments from a couple days ago. Turns out I forgot to hit the Submit review button. Sigh, Bromeon covered most of it though.

src/recipes/editor-plugin/inspector-plugins.md Outdated Show resolved Hide resolved
@snatvb
Copy link
Contributor Author

snatvb commented Jun 12, 2024

You know, I was wondering why you weren't responding to my review comments from a couple days ago. Turns out I forgot to hit the Submit review button. Sigh, Bromeon covered most of it though.

Sorry, it's really hard to read comments that haven't been posted 🤣

@Bromeon Bromeon merged commit 8fdcccb into godot-rust:master Jun 19, 2024
4 checks passed
@Bromeon
Copy link
Member

Bromeon commented Jun 19, 2024

Thanks a lot! 🙂

@snatvb snatvb deleted the inspector-plugin-recipe branch June 22, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-topic New content added to the book
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants