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.

Sign up for GitHub

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

Jump to bottom

Ship window improvements (rebased) #6422

Closed
wants to merge 9 commits into from
Closed

Ship window improvements (rebased) #6422

wants to merge 9 commits into from
+36 −23

Conversation

bunnybot
Copy link

tothxaMirrored from Codeberg
Created on Fri Mar 29 02:17:34 CET 2024 by Tóth András (tothxa)

Type of change
Bugfix & New feature

Issue(s) closed
Replaces #6331 (rebased on master from protected/navalwarfare)

Fixes #6330

New behavior

  • only display SoldierPanel for warships that are ready to fight, show onboard wares display (possibly with soldiers as generic cargo) while refitting
  • allow stopping expeditions where they are (like warships) when they are scouting, only show construct port button when active

Possible regressions
Ship window, wares and workers handling while refitting and soldier capacity is set to 0

Additional context
Expedition stopping is in separate commit(https://codeberg.org/tothxa/widelands/commit/afc16dd84f78a31c3e2aad88c0f3897c58c6c8b2), so it can be reverted easily if voted down. Otherwise icons should be more different probably. Either use scaled down portspace icon for construct port, or a smaller anchor in the stopping icon?

@bunnybot bunnybot added this to the v1.3 milestone Mar 29, 2024
@bunnybot bunnybot self-assigned this Mar 29, 2024
@bunnybot
Copy link
Author

Assigned to tothxa

@bunnybot bunnybot added bug Something isn't working enhancement New feature or request military Soldiers, military sites, training sites & battles seafaring Ships, ports, naval invasions, … accessibility Color contrast, font size, user experience, etc. ui User interface ci:success CI checks succeeded labels Mar 29, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Apr 21, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 1, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 14, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 19, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 30, 2024
@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Thu May 30 23:43:30 CEST 2024, Tóth András (tothxa) wrote:

In my opinion the two icons look different enough.

Here's the expedition window with port_0.5.png copied to ship_construct_port_space.png:

construct_port_scrrenshot.png

I quite like it, though haven't tested in a longer game. But if needed, we now have the blender model, so we can scale it down slightly. Or should I add the downward arrow too?

One small-ish thing that should be fixed: When clicking Stay for an expedition while in sight of a port space, the Build Port button should become available again. Currently the only way to build a port after sailing even a single field away from it is to go very far away until it is out of sight and then return.

OK, I'll try to do it.

@tothxa
fix Ship::exp_scouting_direction() for WalkingDir::IDLE
1915a2c 
re-enables construct port button when ship is stopped while still seeing a portspace
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 30, 2024
@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Fri May 31 02:03:36 CEST 2024, Tóth András (tothxa) wrote:

Oops, I didn't mean to commit the icon change without approval. Sorry!

@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Fri May 31 13:22:24 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Works 👍

One more nit, after giving the command to construct a port, the Stay button remains enabled for a second or so before the ship reverts to a regular Transport ship. Pressing this button at this time results in a log message

WARNING:  1:ship on  25x 32 received scout command but not in kExpeditionWaiting or kExpeditionPortspaceFound or kExpeditionScouting status (expedition: Y), ignoring...

I suppose this could be fixed by disabling the stay button when the ship is colonizing.

I'm not really happy with the new Construct Port icon, IMHO it looks a bit fuzzy and lacking contrast at this scale. Maybe overlay the old anchor icon above it?

@bunnybot bunnybot removed the ci:success CI checks succeeded label May 31, 2024
bunnybot
bunnybot commented May 31, 2024
View reviewed changes
Copy link
Author

@bunnybot bunnybot left a comment

Choose a reason for hiding this comment

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

NordfrieseMirrored from Codeberg
On Fri May 31 16:04:04 CEST 2024, Benedikt Straub (Nordfriese) approved this pull request:

Great, thank you :)

<@>bunnybot merge

@bunnybot bunnybot added the review:approve The pull request has been approved. label May 31, 2024
@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Fri May 31 16:25:33 CEST 2024, Tóth András (tothxa) wrote:

Thank you for the review! :)

Here's the icon's GIMP file for the media repo.

@bunnybot bunnybot added the ci:success CI checks succeeded label May 31, 2024
@Noordfrees
Copy link
Member

@bunnybot merge

@bunnybot bunnybot closed this May 31, 2024
@bunnybot bunnybot deleted the mirror/tothxa/widelands/improve-shipwindow-rebase branch May 31, 2024 18:30
Noordfrees pushed a commit to Noordfrees/widelands that referenced this pull request May 31, 2024
@tothxa
Ship window improvements (rebased) (GH widelands#6422 / CB widelands#…
dce2bfe 
…4784)

Co-authored-by: Tóth András <txa-dev@posteo.hu>
Co-committed-by: Tóth András <txa-dev@posteo.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bunnybot bunnybot

Labels
accessibility Color contrast, font size, user experience, etc. bug Something isn't working ci:success CI checks succeeded enhancement New feature or request military Soldiers, military sites, training sites & battles review:approve The pull request has been approved. seafaring Ships, ports, naval invasions, … ui User interface
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Warship soldier capacity can't be set to 0 if ship still has wares
3 participants
@bunnybot @Noordfrees @tothxa