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

Minimap improvements #5876

Merged
merged 65 commits into from May 7, 2023
Merged

Minimap improvements #5876

merged 65 commits into from May 7, 2023

Conversation

hessenfarmer
Copy link
Contributor

@hessenfarmer hessenfarmer commented Apr 26, 2023

Type of change
New feature

Issue(s) closed
fixes #1781, fixes #2144, fixes #4466, fixes #2074

New behavior
Signal busy roads and flags with more then 5 wares on the minimap in the inverse color

How it works
Determine whether a flag or road is busy and inverse the display color

Possible regressions
Minimap visibilty

Screenshots
grafik

Additional context
things that still need implementation

Copy link
Member

@Noordfrees Noordfrees left a comment

Choose a reason for hiding this comment

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

Looks good :)
Code LGTM, one comment inline

src/graphic/minimap_renderer.cc Outdated Show resolved Hide resolved
@Noordfrees Noordfrees added this to the v1.2 milestone Apr 27, 2023
@Noordfrees
Copy link
Member

Noordfrees commented Apr 27, 2023

But maybe we should not draw this information about roads and flags belonging to enemy players?

@hessenfarmer
Copy link
Contributor Author

But maybe we should not draw this information about roads and flags belonging to enemy players?

We draw only what is in our visibility, imho. So if we would see it in real, near the border we could safely draw it in the minimap as well. But I am open for others opinions on this.

@hessenfarmer hessenfarmer changed the title Traffic minimap Minimap improvements Apr 27, 2023
@hessenfarmer
Copy link
Contributor Author

ok, first problem where I might need some help:

how to identify a building which is attacked

@Noordfrees
Copy link
Member

Buildings do not currently have a concept of "under attack". When a soldier approaches an enemy militarysite, he notifies the site's attack target, which may choose to send out some defenders early and perhaps send a message. But the fact of the attack is not cached anywhere. This would need some non-trivial changes to militarysite and soldier code to implement properly.


Alternatively, how about instead highlighting the positions of soldiers who are currently in the process of attacking or defending something?

for (Widelands::Bob* bob = field->get_first_bob(); bob != nullptr; bob = bob->get_next_bob()) {
	if (bob->descr()->type() == Widelands::MapObjectType::SOLDIER && dynamic_cast<Widelands::Soldier*>(bob)->is_on_battlefield()) {
		color = ...;
		break;
	}
}

@hessenfarmer
Copy link
Contributor Author

Buildings do not currently have a concept of "under attack". When a soldier approaches an enemy militarysite, he notifies the site's attack target, which may choose to send out some defenders early and perhaps send a message. But the fact of the attack is not cached anywhere. This would need some non-trivial changes to militarysite and soldier code to implement properly.

Alternatively, how about instead highlighting the positions of soldiers who are currently in the process of attacking or defending something?

for (Widelands::Bob* bob = field->get_first_bob(); bob != nullptr; bob = bob->get_next_bob()) {
	if (bob->descr()->type() == Widelands::MapObjectType::SOLDIER && dynamic_cast<Widelands::Soldier*>(bob)->is_on_battlefield()) {
		color = ...;
		break;
	}
}

nice idea will try to use that

Copy link
Member

@Noordfrees Noordfrees left a comment

Choose a reason for hiding this comment

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

The high traffic suppression for other players isn't working
grafik

owner is the owner of the current field, you instead want to compare with the player number of the interactive player. If there is no interactive player (i.e. spectator) then showing it for all players is fine.

@hessenfarmer
Copy link
Contributor Author

The high traffic suppression for other players isn't working
owner is the owner of the current field, you instead want to compare with the player number of the interactive player. If there is no interactive player (i.e. spectator) then showing it for all players is fine.

stupid mistake. Thanks for the help. should be fixed now

@Noordfrees
Copy link
Member

This fixed it for flags but the fix needs to be applied to roads as well

@hessenfarmer
Copy link
Contributor Author

sorry I cant concentrate well today

Copy link
Member

@Noordfrees Noordfrees left a comment

Choose a reason for hiding this comment

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

Thanks, working great now :)

@Noordfrees Noordfrees merged commit 7147e59 into master May 7, 2023
32 of 36 checks passed
@Noordfrees Noordfrees deleted the traffic-minimap branch May 7, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants