-
Notifications
You must be signed in to change notification settings - Fork 124
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
Moved code for fog example into standalone modules #826
base: main
Are you sure you want to change the base?
Conversation
My first Ambient PR so I have requested a couple reviews! 👼 |
Good call on splitting them up, but I think the modules might be a bit much (especially because you have to re-import everything) How does it look if you split them up into functions in the same module? I think you could get the same benefit of modularity, but with less duplication. It might also be worth noting that you can do imports within a function: fn lol() {
use ambient_api::core::apps::components::main_scene;
// do something with `main_scene`
} |
I agree with @philpax, the module in module approach seems much more verbose than necessary. Also not sure if you're using this but with VSCode you can easily manage imports by pressing command-dot on anything that is missing an import to include it (not sure what it is in Windows, maybe ctrl-dot), and equivalently it can remove unused imports for you if you click an unused import and press cmd-dot. |
@philpax What do you mean by "especially because you have to re-import everything"? Also I did not know you can import within a function :o (I will be doing this from now on rather than the modules.) |
There are four problems I'm attempting to work around here, but maybe I can solve them in a different way:
Fixed! ctrl+. while multiple imports are selected merges them
|
@droqen Hm I think imports in functions also has the problem that the code just becomes unnecessarily long. You'll have a lot of duplicate imports in the same file since they're imported for each function.
Ctrl+. when it's yellow should show a menu for this. It'll only do it per line which is a bit unfortunate (for typescript there's an option to do it for all imports).
That should work. Can you try just creating a new "clean" ambient project with
Think that should also work, right @philpax? |
Yeah, should just work - it's all part of the |
Yeah, that should definitely work. We should figure out what's going on with your setup at some point... |
Experimentally reorganized the fog example's code into four modules
The purpose of this is to make it easier to comprehend and edit the code at a high level at a glance, see how the first few lines of code the user sees will help them understand what it is doing immediately:
It is also to make the code more re-usable despite the requirement of complex
use
statements; I might want to use the 'fog' from the fog example in my own package, and the encapsulation of all fog-related components in one place, exclusive of all other components (for e.g. the many camera components), means I can copy thefog_lighting
module into my own code with no changes and begin using it immediately: