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 line to remind you to include the player file in your lib #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Relu-Avali
Copy link

@Relu-Avali Relu-Avali commented Jan 18, 2025

After having spent an unreasonable amount of time trying to figure out why my node was not showing up in the editor I feel a line like this would have been a big time saver.

@Bromeon
Copy link
Member

Bromeon commented Jan 19, 2025

Hey, thank you! 🙂

However, the page currently doesn't mention you should write your code in player.rs, so this was an assumption of yours? Maybe we should be explicit that users can use either lib.rs or a dedicated player.rs file, but the sentence

Add the line mod player; to your lib.rs file.

on its own adds more questions than it answers.

@Bromeon Bromeon added the enhancement Ongoing improvements to readability, flow, etc. label Jan 19, 2025
@Relu-Avali
Copy link
Author

Relu-Avali commented Jan 19, 2025

Hey, thank you! 🙂

However, the page currently doesn't mention you should write your code in player.rs, so this was an assumption of yours? Maybe we should be explicit that users can use either lib.rs or a dedicated player.rs file, but the sentence

Add the line mod player; to your lib.rs file.

on its own adds more questions than it answers.

You are correct, I may have jumped too fast on this one. Out of habit I placed all player code on its own player.rs file, to me it was a very natural thing to do.

I would like to pitch it as an idea/suggestion in that case:
Have the documentations make the reader create a player.rs file and move all player code into it. I think makes more sense having it in its own file instead of lib.rs. It would be better clean code practice and also teach the reader that for any node to show in the Godot editor it must be included in the compiled Rust create.

@Bromeon
Copy link
Member

Bromeon commented Jan 19, 2025

I would like to pitch it as an idea/suggestion in that case:
Have the documentations make the reader create a player.rs file and move all player code into it. I think makes more sense having it in its own file instead of lib.rs. It would be better clean code practice and also teach the reader that for any node to show in the Godot editor it must be included in the compiled Rust create.

Since this is a "Hello World" example, we should keep the number of introduced concepts to a minimum, and focus on the important parts to distract too much -- i.e. on how the entry point and classes are defined.

Knowing how Rust modules work is a prerequesite of the book, but I see that it may be worth mentioning just to avoid the confusion you encountered. But it shouldn't be more than a side-note, and it shouldn't end up in code -- users are perfectly within their rights to use lib.rs for everything, especially during Hello World.

As such, I'd extend the existing paragraph in Class declaration:

In this example, we declare a class called Player, which inherits Sprite2D (a node type).
This can be either defined in lib.rs or in a separate file player.rs. In case you go for the latter, don't forget to declare mod player; in your lib.rs file.

What do you think?

@Relu-Avali
Copy link
Author

Since this is a "Hello World" example, we should keep the number of introduced concepts to a minimum, and focus on the important parts to distract too much -- i.e. on how the entry point and classes are defined.

Knowing how Rust modules work is a prerequesite of the book, but I see that it may be worth mentioning just to avoid the confusion you encountered. But it shouldn't be more than a side-note, and it shouldn't end up in code -- users are perfectly within their rights to use lib.rs for everything, especially during Hello World.

As such, I'd extend the existing paragraph in Class declaration:

In this example, we declare a class called Player, which inherits Sprite2D (a node type).
This can be either defined in lib.rs or in a separate file player.rs. In case you go for the latter, don't forget to declare mod player; in your lib.rs file.

What do you think?

I agree that number of new concepts should be kept to a minimum in a hello world example. This all stems from a mistake I made. Am I the first one to create a new file for player code and not include it on the create? If I am the only one I do not think any changes are necessary. If this a reoccurring mistake I agree that your suggested changes are good.

@Bromeon
Copy link
Member

Bromeon commented Jan 19, 2025

Hard to say if this occurs often, but even if not, mentioning it briefly in a sentence (like I suggested) may clear things up.

My point was mostly to not dedicate too much content to "how to structure modules in Rust", because that's a topic that people should already know. So showing how to apply that knowledge is good, but teaching it is a bit too much 🙂

@Bromeon
Copy link
Member

Bromeon commented Feb 2, 2025

So, what do you think about my suggestion earlier?

As such, I'd extend the existing paragraph in Class declaration:

In this example, we declare a class called Player, which inherits Sprite2D (a node type).
This can be either defined in lib.rs or in a separate file player.rs. In case you go for the latter, don't forget to declare mod player; in your lib.rs file.

What do you think?

@Relu-Avali
Copy link
Author

What do you think?

I think it is a good suggestion, I like it.

@Bromeon
Copy link
Member

Bromeon commented Feb 5, 2025

I meant, do you want to update your PR to reflect this? 😉

@Relu-Avali Relu-Avali force-pushed the add_player_file_to_lib branch from bdc4bf2 to 6b18b17 Compare February 6, 2025 10:30
@Relu-Avali Relu-Avali force-pushed the add_player_file_to_lib branch from 6b18b17 to 9b22878 Compare February 6, 2025 10:32
@Relu-Avali
Copy link
Author

I have updated the PR based on previous discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ongoing improvements to readability, flow, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants