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

ls (done) and scn commands. #6

Open
tryoxiss opened this issue Jan 15, 2023 · 9 comments
Open

ls (done) and scn commands. #6

tryoxiss opened this issue Jan 15, 2023 · 9 comments
Labels
good first issue Good for newcomers new command This adds a new command proposal Suggest a change or a new feature

Comments

@tryoxiss
Copy link
Contributor

scn is used to edit the current scene (e.g., instancing nodes, freeing nodes). ls is used to view the current scene tree. I have some docs articles here to give an idea of what i'm thinking but feel free to suggest changes!

@Kubulambula Kubulambula added the proposal Suggest a change or a new feature label Jan 15, 2023
@Kubulambula Kubulambula added the good first issue Good for newcomers label Mar 6, 2023
@Kubulambula
Copy link
Owner

The ls command is great especially with the #15 proposal.

With the proposal however, I would split the scn command into separate specialized commands like instance and free instead of parameters.
I'm not sure if we should do the kill command then or just to have a flag for free to use free() instead of queue_free() - discussion for the future.

@Kubulambula Kubulambula added the new command This adds a new command label Mar 30, 2023
@tryoxiss
Copy link
Contributor Author

tryoxiss commented Apr 2, 2023

I defintely agree with the seccond point, there could be like a --now or -N flag.

For the first one I defintely see what your saying, and thinking about it now I do think I agree with it. Maybe call it instance or ins for short maybe? Maybe its just a me thing but I don't like typing out commonly used commands a lot when they are long, so shorter aliases are worth thinking about I think.

@Kubulambula
Copy link
Owner

  1. The --now or -N flags are not really that descriptive about what it does. Maybe the free command would do free() by default and with --queue or -Q flag it would do queue_free()?

  2. Well we could call it instance and have it register with an ins alias. That's not really an issue.

@tryoxiss
Copy link
Contributor Author

tryoxiss commented Apr 3, 2023

I think --now is fairly descriptive, and we defintely don't want free() to be the default as queue_free() will be used a lot more and is what should be used. Maybe --ignore-active-actions or something for free() instead of queue_free()? Not sure.

I think I am going to try and make these, not sure how far I will get though. Nevermind, literally nothing works. Not even the default UI scene is valid apparently (according to both godot 3.5 and 4.0).

@Kubulambula
Copy link
Owner

I think --now is fairly descriptive, and we defintely don't want free() to be the default as queue_free() will be used a lot more and is what should be used. Maybe --ignore-active-actions or something for free() instead of queue_free()? Not sure.

I definitely agree that queue_free() should be the default behavior. Also note that Object does not have the queue_free() method as it is on the inherited Node class. This might cause some issues in the future.

But for the flag: --now is definitely better than having free() as the default. Maybe we could use --force, --unsafe, or just simply --free with --queue_free and --queue_free will be implicit default?

I don't know honestly. Maybe we should stick with --now


Nevermind, literally nothing works. Not even the default UI scene is valid apparently (according to both godot 3.5 and 4.0).

What do you mean? I just downloaded the main branch and it works fine for me in 4.0.stable

Can you share the error?

@tryoxiss
Copy link
Contributor Author

tryoxiss commented Apr 3, 2023

Yeah I think --force and --unsafe are defiontely better than what I got! and yeah queue_free being the implcit default would be good.

Well is there anything stopping us from having multiple flags mean the same thing? I think now, force, and unsafe are all good and descriptive enough and its hard to chose any one.


Ahh, I added it to an existing project so I could more easily test the scenes and assumed it wouldn't have issues, though I don't think the errors are related to that. (Looking at the format, I don't even think it would work with just that as theres no project.godot), I also created a new project with just that and got the same errors.

Errors:

  1. Unknwon class: "GDShellCommand"
  2. plugin.gd: Parse error: Unexpected "@"
  3. plugin.gd (when I remove that): the identifier GDSHellMain isn't declared in the current scope
  4. default_ui.tscn: Error while parsing default_ui.tscn

@Kubulambula
Copy link
Owner

Yeah I think --force and --unsafe are defiontely better than what I got! and yeah queue_free being the implcit default would be good.

Well is there anything stopping us from having multiple flags mean the same thing? I think now, force, and unsafe are all good and descriptive enough and its hard to chose any one.

Good idea! I think we could have --safe and --unsafe with --now or --force as an alias for --unsafe as it is not a default.


Ahh, I added it to an existing project so I could more easily test the scenes and assumed it wouldn't have issues, though I don't think the errors are related to that. (Looking at the format, I don't even think it would work with just that as theres no project.godot), I also created a new project with just that and got the same errors.

Yeah the project.godot is in .gitignore so if you want to run GDShell project itself, you have to create it manually (I should probably change that)

But the errors are very strange. Maybe try to delete the .godot/ directory inside your project and let Godot to reimport all the classes. Also what Godot version are you using?

@CollCaz
Copy link
Contributor

CollCaz commented Aug 11, 2023

So i tried implementing the ls command and, couldn't find a way to see if a node is an instanced scene or just a "stray" node.
Any idea how i could do that?

@tryoxiss
Copy link
Contributor Author

So i tried implementing the ls command and, couldn't find a way to see if a node is an instanced scene or just a "stray" node. Any idea how i could do that?

I believe there is a is_instance() method, but that may be my imagination. A ask.godotengine.org thread suggests trying the get_filename() method, if its a scene it returns a file, if its a stray node it returns none if I am understanding correctly.

Best luck on implementing!

@Kubulambula Kubulambula changed the title ls and scn commands. ls (done) and scn commands. Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers new command This adds a new command proposal Suggest a change or a new feature
Projects
None yet
Development

No branches or pull requests

3 participants