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

Search bar for save/load window and crashes fix #5634

Conversation

Max5377
Copy link
Contributor

@Max5377 Max5377 commented Sep 24, 2023

Search bar for save/load window with some rewrite

This PR adds search bar for save/load window to search for specific save by name for quickly choosing it from the list, if player has a lot of saves, for example. Some rewrite to better support resolution scaling and addition of new elements to bottom part.

In action:

Pioneer.search.bar.for.save.load.menu.mp4

Known issue: although not showed in the video, doesn't support case-insensitive search for non-english characters due to #4110

And some fixes to two crashes that don't have their own issue.

Crash 1

Crash 1 message

This was happening in main menu with opened tab Commodity Price in Debug Info, because it lacked check in registerTab in DebugCommodityPrices:
if Game.player == nil then return end

Crash 2

Crash 2 message

This was happening when player tried to choose new game variant in new game window after loading any save and then exiting to main menu.
This was happening, because in Equipment.lua tables that define items are reinitialized after loading the save.
This tables is used during registering of start variants in new-game-window\class.lua (ex. for Start At Mars start):

StartVariants.register({
	name       = lui.START_AT_MARS,
	desc       = lui.START_AT_MARS_DESC,
	location   = SystemPath.New(0,0,0,0,18),
	logmsg     = lui.START_LOG_ENTRY_1,
	shipType   = 'coronatrix',
	money      = 600,
	hyperdrive = true,
	equipment  = {
		{ laser.pulsecannon_1mw,      1 },
		{ misc.atmospheric_shielding, 1 },
		{ misc.autopilot,             1 },
		{ misc.radar,                 1 }
	},
	cargo      = {
		{ Commodities.hydrogen, 2 }
	},
	pattern    = 1,
	colors     = { Color('000000'), Color('000000'), Color('000000') }
})

and then this tables are compared in this function in new-game-window\ship.lua:

local function findEquipmentPath(eqType)
	for _, eq_list in pairs({ 'misc', 'laser', 'hyperspace' }) do
		for id, obj in pairs(Equipment[eq_list]) do
			if obj == eqType then
				return eq_list, id
			end
		end
	end
	assert(false, "Wrong Equipment ID: " .. tostring(eqType))
end

But since tables in Equipment.lua are reinitialized on save load the game will crash with message showed earlier.
So comparison changed to compare against equipment's l10n_key since it's unique for every equipment.

Fixes crash with this tab opened when in main menu.
Fixes crash when choosing new game variant after loading the save and exiting back to main menu.
@impaktor
Copy link
Member

Search bar looks nice.
First bug is on me. Fix looks good.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

I'd suggest that the search bar would work much better as a separate element before the list of save files (with the case-sensitive checkbox on the same line as the search box). With the search bar on the bottom, the difference in placement for the filename input vs. search bar in the save/load menu is very awkward and can be confusing for a new user. It's also a UX issue to have the search bar below the Save/Load buttons, as those should semantically be the last items in the window.

Other than that change, there are a few code style changes I'd like to see made before I approve this PR, but on the whole I'm pretty happy with it.

data/pigui/modules/saveloadgame.lua Outdated Show resolved Hide resolved
data/pigui/modules/saveloadgame.lua Outdated Show resolved Hide resolved
data/pigui/modules/saveloadgame.lua Outdated Show resolved Hide resolved
@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

This is how it looks like right now:

Load mode

SearchBarSaveLoadWindow(Load)

Save mode

SearchBarSaveLoadWindow(Save)

The only issues right now is buttons are shifting more to separator in load mode at the bottom the lower the resolution is. And there's an additional scroll bar at low resolution too. Besides that I think it works fine.

Also in getSaveTooltip(name) there's a lot of elseif statements:

if stats.flight_state == "docked" then ret = ret .. lc.DOCKED
elseif stats.flight_state == "docking" then ret = ret .. lc.DOCKING
elseif stats.flight_state == "flying" then ret = ret .. lui.FLYING
...
else ret = ret .. lc.UNKNOWN end 

maybe we can change this to table:

table["docked"] = lc.DOCKED
table["docking"] = lc.DOCKING
-- more flight states here

And change this statements to this:
ret = ret .. table[stats.flight_state] or lc.UNKNOWN

I think this will make code look more clean. And make it easier to add additional checks if you planning to add more flight states in the future.

What do you think about it before I make a force-push?

@Web-eWorks
Copy link
Member

The only issues right now is buttons are shifting more to separator in load mode at the bottom the lower the resolution is. And there's an additional scroll bar at low resolution too. Besides that I think it works fine.

You can start the line with the "Save" text and place the filename bar on the same line as the Save text. Same with the "Search" text at the top if you feel so inclined. At low resolutions, the save file dialog should be modified to fill most of the screen.

Also in getSaveTooltip(name) there's a lot of elseif statements:

As long as the enum strings are the same as the lang strings but in a different case, you could simply do ret = rawget(lc, string.upper(stats.flight_state)) or lc.UNKNOWN. That transforms "docking" -> "DOCKING" and falls back to the unknown string instead of the auto-generated placeholder for untranslated lang strings. If that's not the case, then you'll have to maintain a lookup table instead, yes.

Search bar for save/load window to search saves by name with a checkbox to make search case-sensitive. Some rewrite to make this window more prone to resolution scaling.
Case-sensitive text entry for checkbox in save/load window
@Max5377 Max5377 force-pushed the SaveLoadWindowSearchBar-and-CrashesFix branch from 3ce656f to ac9f7ea Compare September 27, 2023 20:21
@Web-eWorks
Copy link
Member

By the by, looking at the code - you only need to add the CASE_SENSITIVE string to en.json in the language resource you're adding it to - Transifex will initialize the other languages in the resource with the English value once the PR has been merged (and the game will internally fall back to the en version if it's defined when you're developing).

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

I added this to every lang in ui-core with RU translation. In an additional commit.

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

Also, about buttons and additional scroll bar. I just wanted to know if you wanted it to be fixed. I don't care myself, because I don't play with this resolution and I don't know anybody who plays on it nowdays. This window should look fine on resolutions from 1280x1024 to 2560x1440, but feel free to tell me if I missed something. Thanks for letting me know about rawget, didn't know about it. I added multiple rawget's, because flight-states stored in core and ui-core.
Edit: also will do additional PR with fixes to this issues #5557 #5577 #5534 tommorow.
Edit2: didn't know about fallback to en, it will make my life easier next time.

@Web-eWorks
Copy link
Member

Also, about buttons and additional scroll bar. I just wanted to know if you wanted it to be fixed. I don't care myself, because I don't play with this resolution and I don't know anybody who plays on it nowdays.

Pioneer unofficially targets a minimum resolution of 1280x720, so as long as it works on this resolution or larger it should be fine. I've not gotten a chance to test it yet, but even if there are scalability issues they can be fixed in a separate PR.

We should probably make this an official minimum resolution, but we haven't made the decision to drop the potential for running the game at 800x600 (albeit with UI issues) yet.

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 28, 2023

I checked, works fine on 1280x720.
Edit:
Load:
SearchBarSaveLoadWindow(Load)

Save:
SearchBarSaveLoadWindow(Save)

@impaktor
Copy link
Member

I added this to every lang in ui-core with RU translation. In an additional commit.

I'm not sure that (RU) translation will persist, might be over-written by transifex. Maybe not, perhaps.

Btw, if you want to play more with these screens, perhaps we want the ability to delete saves as well? (definitely not for this PR)

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 29, 2023

@impaktor Yeah, I can do that, but later. Right now waiting for review, I think that I will need to refine a lot 😁

- Use ImGui tooltip delay function to avoid parsing the savefile unless the user really wants to see the hover tooltip
- Expose SetItemTooltip / ImGuiHoveredFlags functionality
Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

I've pushed one change to reduce the lag when scrolling/hovering the list of saved games, but otherwise I'm happy with this PR. Good work!

@Web-eWorks Web-eWorks merged commit 1139778 into pioneerspacesim:master Oct 8, 2023
4 checks passed
@Max5377 Max5377 deleted the SaveLoadWindowSearchBar-and-CrashesFix branch October 8, 2023 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants