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

Moved code for fog example into standalone modules #826

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

droqen
Copy link

@droqen droqen commented Sep 8, 2023

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:

[..]
#[main]
fn main() {
    default_camera::init();
    ground_and_cubes::init();
    let sun = fog_lighting::spawn_sun();
    ui_sliders_for_sun::init(sun);
}

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 the fog_lighting module into my own code with no changes and begin using it immediately:

mod fog_lighting {
    use ambient_api::{
        core::{
            app::components::main_scene,
            rendering::components::{
                fog_color, fog_density, fog_height_falloff, light_diffuse, sky, sun,
            },
            transform::{components::rotation, concepts::make_transformable},
        },
        prelude::*,
    };
    pub fn spawn_sun() -> EntityId {
        let sun = Entity::new()
            .with_merge(make_transformable())
            .with(sun(), 0.0)
            .with(rotation(), Quat::from_rotation_y(-1.))
            .with(main_scene(), ())
            .with(light_diffuse(), Vec3::ONE)
            .with(fog_color(), vec3(1., 1., 1.))
            .with(fog_density(), 0.1)
            .with(fog_height_falloff(), 0.01)
            .spawn();

        Entity::new()
            .with_merge(make_transformable())
            .with(sky(), ())
            .spawn();

        sun
    }
}

@droqen
Copy link
Author

droqen commented Sep 8, 2023

My first Ambient PR so I have requested a couple reviews! 👼
A very simple one, but it may have some bigger implications style-wise. (Not directly related but #766-adjacent?)

@philpax
Copy link
Contributor

philpax commented Sep 11, 2023

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`
}

@FredrikNoren
Copy link
Contributor

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.

@droqen
Copy link
Author

droqen commented Sep 11, 2023

@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.)

@droqen
Copy link
Author

droqen commented Sep 11, 2023

There are four problems I'm attempting to work around here, but maybe I can solve them in a different way:

  • Combining multiple import statements. or adding needed imports all at once --> I am more hopeful for combining multiple import statements, but if I could just run a 'fix all my unimported stuff' command that would be great! At the moment there are lots of tiny messes that are OK individually, but become a real irritation cumulatively, that look like this:
use thing::{a, b, c: {c1, c2, c3::{c3a, c3b}}};
function_i_want() {
   c3b
}
other_function_i_dont_want() { .. }
and_another_one() { .. }
man_there_are_a_lot_of_these() { .. }
//etc

Fixed! ctrl+. while multiple imports are selected merges them


  • Cleaning up unused imports all at once --> Maybe there is a command or package for doing this

  • Ctrl+. does not work for my packages' components --> As I have come to understand it, this may actually a problem with my config?

  • Ctrl+. does not work for remote packages' components --> I don't think this is possible yet, but maybe I underestimate the magic

@FredrikNoren
Copy link
Contributor

@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.

Cleaning up unused imports all at once --> Maybe there is a command or package for doing this

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).

Ctrl+. does not work for my packages' components --> As I have come to understand it, this may actually a problem with my config?

That should work. Can you try just creating a new "clean" ambient project with ambient new and see if it works there?

Ctrl+. does not work for remote packages' components --> I don't think this is possible yet, but maybe I underestimate the magic

Think that should also work, right @philpax?

@philpax
Copy link
Contributor

philpax commented Sep 12, 2023

Yeah, should just work - it's all part of the packages module, which your code has access to.

@droqen
Copy link
Author

droqen commented Sep 12, 2023

It is not working even in a "clean" ambient project.

image

image

@droqen
Copy link
Author

droqen commented Sep 12, 2023

Autocompleting 'packages' does not work:

image

Some things work by VS Code 'guessing' at me possibly wanting to type a word that I've seen before, but it has no context so it autocompletes things in wrong places:

image

@philpax
Copy link
Contributor

philpax commented Sep 12, 2023

Yeah, that should definitely work. We should figure out what's going on with your setup at some point...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants