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

higher dezoom + m hotkey to load another map #223

Closed
wants to merge 3 commits into from

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Nov 2, 2022

worked on top of !225

Comment on lines 164 to 170
let path = Path::new("./de_map_test.tar");
let tmp_dir = Builder::new().prefix("de_map_").tempdir().unwrap();
let mut tmp_dir_path = PathBuf::from(tmp_dir.path());
tmp_dir_path.push("test-map.tar");

task::block_on(store_map(&map, tmp_dir_path.as_path())).unwrap();
let loaded_map = task::block_on(load_map(tmp_dir_path.as_path())).unwrap();
task::block_on(store_map(&map, &path)).unwrap();
let loaded_map = task::block_on(load_map(&path)).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was only done to create a fake alternative map ; those changes don't belong there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done the same trick. Another PR which extends the de_tools crate with some kind of simple "generate map" functionality would be appreciated.

@@ -23,13 +23,13 @@ const CAMERA_HORIZONTAL_SPEED: InverseSecond = Quantity::new_unchecked(2.0);
/// Minimum camera distance from terrain achievable with zooming along.
const MIN_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(20.);
/// Maximum camera distance from terrain achievable with zooming alone.
const MAX_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(100.);
const MAX_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(300.);
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 personally think those are better defaults (for now at least), as the default map is way too big and needs a long scroll to see other units

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, at this point in time this change makes sense.

A bit of a context:

  • In the long run, I would like to implement a custom LOD so zooming out a lot is feasible from performance perspective.

    • Until this happens, it might be necessary to change this back to a lower value after we plug in better unit models (with possibly higher triangle counts etc.).
  • The appearance / UI has to change on high camera distances: drawing simple colored squares instead of true 3D models, not displaying health bars and selection circles etc. More info here LOD, "cartographic rendering", split screen & full-screen minimap #153.

  • Implementing some kind of a mini map (displaying at least all units & buildings & camera) is high on my priority list. That would solve most of the issue you mention (long scroll -> single click on the mini map, getting disoriented -> looking where you are looking at at the mini map).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is related #68.

@@ -48,6 +51,7 @@ fn spawn_map(
task: Option<ResMut<MapLoadingTask>>,
mut move_focus_events: EventWriter<MoveFocusEvent>,
game_config: Res<GameConfig>,
entities_to_remove: Query<Entity, Or<(With<Handle<Scene>>, With<RemoveBeforeLoad>)>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might want a better strategy for removing stuff between loading states, I would err on the side of tagging everything that should not be removed (as I guess there should be a less amount of entities) rather than the opposite, but that was easier to implement.

I might have missed edge cases like the selection rectangle, or projectiles ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the desired solution to this problem is to add on exist system(s) to every plugin so that the responsibility is moved to where the entities / resources were inserted in the first place.

This all thing might not be needed to add map selection (see the other comment): I would just implement simple main menu where you click on a button with a map name to start the game. You would have to restart the game to select another map to decouple proper system clenaups with map selection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related #135.

Copy link
Collaborator

@Indy2222 Indy2222 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! It is the first contribution from anyone but me. I really appreciate it.

I see that this is still a draft but I have reviewed it anyways to give you some feedback ASAP.

@@ -23,13 +23,13 @@ const CAMERA_HORIZONTAL_SPEED: InverseSecond = Quantity::new_unchecked(2.0);
/// Minimum camera distance from terrain achievable with zooming along.
const MIN_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(20.);
/// Maximum camera distance from terrain achievable with zooming alone.
const MAX_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(100.);
const MAX_CAMERA_DISTANCE: Metre = Quantity::new_unchecked(300.);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, at this point in time this change makes sense.

A bit of a context:

  • In the long run, I would like to implement a custom LOD so zooming out a lot is feasible from performance perspective.

    • Until this happens, it might be necessary to change this back to a lower value after we plug in better unit models (with possibly higher triangle counts etc.).
  • The appearance / UI has to change on high camera distances: drawing simple colored squares instead of true 3D models, not displaying health bars and selection circles etc. More info here LOD, "cartographic rendering", split screen & full-screen minimap #153.

  • Implementing some kind of a mini map (displaying at least all units & buildings & camera) is high on my priority list. That would solve most of the issue you mention (long scroll -> single click on the mini map, getting disoriented -> looking where you are looking at at the mini map).

Comment on lines 164 to 170
let path = Path::new("./de_map_test.tar");
let tmp_dir = Builder::new().prefix("de_map_").tempdir().unwrap();
let mut tmp_dir_path = PathBuf::from(tmp_dir.path());
tmp_dir_path.push("test-map.tar");

task::block_on(store_map(&map, tmp_dir_path.as_path())).unwrap();
let loaded_map = task::block_on(load_map(tmp_dir_path.as_path())).unwrap();
task::block_on(store_map(&map, &path)).unwrap();
let loaded_map = task::block_on(load_map(&path)).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done the same trick. Another PR which extends the de_tools crate with some kind of simple "generate map" functionality would be appreciated.

@@ -209,7 +209,7 @@ impl DesiredPoW {
}
}

fn setup(mut commands: Commands) {
fn setup(mut commands: Commands, camera_query: Query<Entity, With<Camera3d>>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will have to thing about it a bit more, but my current thought is that the whole map cycling & game setup has to be done differently:

  • Proper clean up after GameState::Playing is existed has to be implemented.
  • Map selection should be done via main menu. It might be very simplistic at this point (a button for each available map?).
  • Generally: once you enter a game (GameState::Playing) you cannot change its configuration (like in most RTS games). You have to quit the game first and then start a new game.

My second point is of highest priority IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related #34.

@@ -48,6 +51,7 @@ fn spawn_map(
task: Option<ResMut<MapLoadingTask>>,
mut move_focus_events: EventWriter<MoveFocusEvent>,
game_config: Res<GameConfig>,
entities_to_remove: Query<Entity, Or<(With<Handle<Scene>>, With<RemoveBeforeLoad>)>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the desired solution to this problem is to add on exist system(s) to every plugin so that the responsibility is moved to where the entities / resources were inserted in the first place.

This all thing might not be needed to add map selection (see the other comment): I would just implement simple main menu where you click on a button with a map name to start the game. You would have to restart the game to select another map to decouple proper system clenaups with map selection.

@Indy2222
Copy link
Collaborator

Indy2222 commented Nov 3, 2022

Also, please mention this issue in relevant commits: #70

@Vrixyz Vrixyz force-pushed the 124-hotkey-cycle-map branch 4 times, most recently from bd839d0 to c3b6e8e Compare November 3, 2022 20:39
@Indy2222
Copy link
Collaborator

Indy2222 commented Dec 9, 2022

Superseded by #259.

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

Successfully merging this pull request may close these issues.

2 participants