Police to pigui

[impaktor]

Introduction

This is my first pigui contribution. I hope to get some eyes on this, there's at least one "breaking issue" that needs fixing before merge (see below), that I haven't managed to figure out.

Other than that, it has all the same functionality as the original, which wasn't very flashy to begin with, since that was also something I barely managed to hack together.

Description

So I'm sure the data here can be presented better, (e.g. remove gray color for crime record, use some collapsing header or similar instead), but the question is how much more work should be done before this is merged.

My plan was to do another PR after this where I add some sort of dialogue button as well, so the player can ask for missing persons (needed for #4746), but if this PR looks OK, I could attack the ShipRepair screen next, which should be trivially similar to this.

I've made a little Debug-module that allows for adding crime and money to the player's record that's useful for testing this PR, if anyone wants to play (it also allows playing with hull damage, swap ships, adjust reputation and killcount). See Appendix-section below.

Know issues

1. When trying to pay fines higher than what the player has, the modal popup window saying "not enough money" crashes the game when clicking "OK". I'm not sure what the correct way of implementing this is. Pointers are most welcome, ping @vakhoir

2. widgetSizes table is a copy-paste from lobby, that I haven't put that much thought into. Ideally I should rename it (and think through what it's doing)

3. I'm not sure how pioneer-style compliant I am with colors, etc. Please point out if it's too uncceptably deviant.

4. edit also, I added a as a separate commit the faction name and police faction name. Not sure we want this, if we want it I'll add it to translation system and squash the commit

Screenshots

Compare below with original shown in #3254

Appendix: Debug-window

Drop into data/modules/ adds a window on worldview.

-- Copyright © 2008-2020 Pioneer Developers. See AUTHORS.txt for details
-- Licensed under the terms of the GPL v3. See licenses/GPL-3.txt

--[[
	data/pigui/modules/add_crime.lua

	Written for @impaktor as a tutorial on how to get started with pigui by @sturnclaw, extended by @impaktor
--]]

local Game = import('Game')

local Format = import "Format"

-- pigui is traditionally imported as 'ui' for simplicity.
local ui = require 'pigui'
-- gameView is needed to dispatch to our Hello World window
local gameView = require 'pigui.views.game'
-- we want all displayed text to be localized
local lui = require 'Lang'.GetResource('ui-core')

-- ui.WindowFlags is an acceleration structure to allow composing window flags
-- once and reusing them with each call.
local window_flags = ui.WindowFlags {
	-- We don't want to be able to collapse the window.
	"NoCollapse"
}


local amount = 1000
local selected = 0

-- a small function, to add some offences to player,
-- (for testing ui layout of police tab)
local Legal = import "Legal"
local utils = import "utils"
local Lang = import 'Lang'
local l = Lang.GetResource("ui-core")

-- build list of all crime types:
local crime_types = {}
for k, v in pairs(Legal.CrimeType) do
	table.insert(crime_types, k)
end

local Character = import("Character")

-- build list of all ships
local ShipDef = import("ShipDef")
-- local shipdefs = utils.build_array(utils.filter(function (k,def) return def.tag == 'SHIP' end, pairs(ShipDef)))
local selected_ship = 0
local changed_ship = false

local shipdefs = {}
for k,v in pairs(ShipDef) do
	if v.tag == "SHIP" then
		table.insert(shipdefs, v.id)
	end
end


-- Register this window as a Game-view module. It will be displayed when the
-- player is in the World View.
-- registerModule takes two parameters, the unique key of the module, and the
-- module descriptor table.
gameView.registerModule("add-crime", {

	-- Pretty self explanatory, if this is true, draw() is called when the
	-- ship is in hyperspace as well as in normal space.
	ShowInHyperspace = true,

	-- Called once per frame while the module is active. The function is passed
	-- the module object (this table) and the frame delta time
	draw = function(self, deltaTime)
		-- ui.window takes three parameters: the window title, the window flags,
		-- and a function containing the body of the window.

		-- the window title is an IMGUI string ID - two windows cannot share
		-- the same ID at the same stack position. To work around this,
		-- everything after ## is not part of the title, but instead used to
		-- make the window ID unique.
		ui.window("Debug!##id42", window_flags, function()

			if ui.collapsingHeader("Money", {"DefaultOpen"}) then
				-- args: label, default, min, max, (optional: str format)
				amount = ui.sliderFloat("Amount", amount, 0, 1000000)
				if ui.button("Give money", Vector2(100, 0)) then
					Game.player:AddMoney(amount)
				end
				ui.sameLine()
				ui.text(Format.Money(Game.player:GetMoney()))
			end

			ui.separator()

			if ui.collapsingHeader("Crime", {"DefaultOpen"}) then
				local changed, ret = 0

				ui.text("ADD CRIMINAL CHARGES:")
				local selected_crime = 0
				for i, v in pairs(crime_types) do
					if ui.selectable(v:lower(), selected_crime == i, {}) then
						Legal:notifyOfCrime(Game.player, crime_types[i])
					end
				end

				ui.dummy(Vector2(0,10))
				-- CRIME
				local crimes, fine = Game.player:GetCrimeOutstanding()
				if #utils.build_array(pairs(crimes)) > 0 then
					ui.text(l.OUTSTANDING_FINES)
					for k,v in pairs(crimes) do
						ui.text(v.count.."\t"..Legal.CrimeType[k].name)
					end
					ui.text("Fine:\t".. tostring(fine))
				end

				ui.dummy(Vector2(0,10))
				local past_crimes, _ = Game.player:GetCrimeRecord()
				if #utils.build_array(pairs(past_crimes)) > 0 then
					ui.text(l.CRIMINAL_RECORD)
					for k,v in pairs(past_crimes) do
						local s = v.count.."\t"..Legal.CrimeType[k].name
						ui.text(s, Vector2(100, 0))
					end
				end
			end

			-- Ship
			if ui.collapsingHeader("Ship", {}) then
				changed_ship, idx = ui.combo("Ships", selected_ship, shipdefs)
				if changed_ship then
					selected_ship = idx
					local i = selected_ship + 1
					Game.player:SetShipType(shipdefs[i])
				end

				-- hyperdrive maintenence
				local hyperdrive = Game.player:GetEquip('engine',1)
				if hyperdrive then
					ui.text(hyperdrive.l10n_key)
				end

				local hull = ui.sliderInt("Hull", Game.player.hullPercent, 0, 101)
				Game.player:SetHullPercent(hull)
			end

			-- Reputation & kills
			if ui.collapsingHeader("Reputation & kills", {}) then
				Character.persistent.player.reputation = ui.sliderInt("Reputation", Character.persistent.player.reputation, 0, 512)
				ui.sameLine()
				ui.text(Character.persistent.player:GetReputationRating())

				Character.persistent.player.killcount = ui.sliderInt("Kills", Character.persistent.player.killcount, 0, 6000)
				ui.sameLine()
				ui.text(Character.persistent.player:GetCombatRating())
			end

		end)
	end
})

[vakhoir]

widgetSizes table is a copy-paste from lobby, that I haven't put that much thought into. Ideally I should rename it (and think through what it's doing)

I don't think you need to rename it, I use it with than name with a lot of views. What I'm not sure is if usingceil on all the vectors is necessary. I think I tried it out to align the widgets better but I think it might not be needed.

In any case it's just a local variable for storing widget sizes, for consistency, and so they don't need to get recalculated every frame, etc.

I'm not sure how pioneer-style compliant I am with colors, etc. Please point out if it's too uncceptably deviant.

Maybe we could use a slightly lighter shade of grey for the criminal record, but otherwise looks good.

[impaktor]

Thanks for review!

I've:

• Fixed popup-bug

• Made gray tex a bit brighter / visible (from (76,76,76) to (100,100,100), where 255=white)

• Removed "ceil" call, seems to work fine. My goal with the police tab was to do as little copy-paste as possible from previous pigui screens, so I understand all that's going on in the code, so this is good for me.

• I've put top/first greeting sentence into translation system @nozmajner: do we want that commit? Problem here is that faction names and faction police name is not translatable, as they're pulled from the factions/.json stuff. Still as separate commit, in case we don't want it.

Example:

[Web-eWorks]

@impaktor looking good, I like what you've done with the previous criminal record! A couple of tips / suggestions for you:

• Increase the padding between the offense count and the name of the offense. Right now e.g. Murder is offset, and it spoils the even, table-like look of the display. You could do it like so (with variable names as appropriate):

local countPlusPadding = 40 -- or however much space you want for the count and padding
ui.sameLine(countPlusPadding - ui.calcTextWidth(offenseCountText))
ui.text(offenseName)

• Add an icon to past criminal offenses to indicate that it has been paid off. This could be a checkmark, a money symbol, whatever, but it should indicate that your criminal record has been dealt with.
• There should be a bit more padding between the "this is the police" text and the "Outstanding fines" heading. (Speaking of, "fines" in that heading should be capitalized!)
• The "Pay" button and the money count is misaligned, but that's not something that's easy to fix as a novice (I don't even know the trick to that one, and I definitely don't have the time to go digging right now).

Otherwise, this looks pretty good! I'll try to go over the code tomorrow and see about merging.

As an aside @vakhoir can you do something about the need to load all of the newUI screens to have proper tab ordering? From what I'm looking at, if I start deleting some but not all of the newUI station view screens it'll mess up the pigui-to-newUI hosting because we rely on tab indicies to properly display the correct newUI screen.

[impaktor]

thanks for review and pointers, @Web-eWorks!

• Regarding padding, I intentionally gave my self > 9 offences to see what it looked like, and if I would get complaints 😈. But there's no ui.calcTextWidth so I tried ui.sameLine(countPlusPadding - ui.calcTextSize(v.count).x), but that led to mayhem, and I'm too low energy to figure that out at the moment. Would it not be more natural to use the ui.columns, assuming that aligns width of the columns properly to begin with?

• I think I'll ignore adding icon for now, if that's OK, as I'm not sure we have a fitting one, and this screen will be re-worked quite a bit soonish, by me, and I cant see it looking good, in my minds eye.

• Added more padding between line 1 and 2, as suggested, but will not capitalize "... fines" or "... record", as those are strings already in the translation system, plus it's simply a matter of style, although I know US English ("Chicago Manual") likes to capitalize every word in a heading.

• I've moved the "Pay" button to its own line, thus the difficult alignment issue is "fixed".

• Fixed indentation for line-broken argument lists and functions (my Lua-indentation mode doesn't do as we have agreed it should be done)

[impaktor]

Well, using ui.columns to build a table, gives me:

I'm out of energy for this weekend now.

[Web-eWorks]

@impaktor here's a suggestion of how to make sure your text and offense counts align. This particular suggestion should be able to be automatically merged with no issues, but you might want to move that 40 to its own file-local variable.

In general, I'd recommend you avoid the use of tab characters for spacing and handle the alignment via ui.sameLine() in conjunction with ui.calcTextSize().

EDIT: hrmm, I didn't see that you'd already addressed this particular suggestion; in theory this should work perfectly fine although I'll leave it up to you to find what looks best. There's a function ui.setColumnWidth() that you can use to adjust the alignment of individual columns if that's the route you decide to go down.

[impaktor]

Thanks for the code; but the way I read the source: ui.sameLine(pos_x, spacing_w) it's enough to just give pos_x-coordinate on screen where to start the column? Seems to work fine, see screenshot below. (the code you posted inversed the problem, moving the text too much to the left if the number to the left of it was high, probably becuase the width some some variable wasn't scaled correctly, or something).

However, I've only tested this on the resolution I use (1600x900), so it sounds reasonable that I should make use of some ui.rescaleUI, alas I don't understand that code. If I send in a number, and a resolution, I'd expect to get something similar, but rescaled, back. But I get something x1000 times bigger.

Well, the code path for ui.rescaleUI for my example (sending a number) would be:

ui.rescaleUI = function(val, baseResolution)
	local rescaleVector = Vector2(pigui.screen_width / baseResolution.x, pigui.screen_height / baseResolution.y)
	local rescaleFactor = math.min(rescaleVector.x, rescaleVector.y)
        return val * rescaleFactor
end

In summary: At the moment I don't quite understand what/how it is used, although the name of the function sounds like it's something useful.

(also I squashed the commits to one. (One is legion, we are unity)

So what remains is:

1. Am I wrong in thinking I don't need any calcTextSize? The vaiability of one-two characters between different resolutions might not be that big? (famous last words?)
2. Do I need / how do I use rescaleUI

Status:

[vakhoir]

However, I've only tested this on the resolution I use (1600x900), so it sounds reasonable that I should make use of some ui.rescaleUI, alas I don't understand that code. If I send in a number, and a resolution, I'd expect to get something similar, but rescaled, back. But I get something x1000 times bigger.

I might need to rename the parameters, but I can't think of anything better. Or maybe I'll just add a comment explaining how it works. The idea is that you pass the current resolution you're working with, and the size of the widget has at that resolution, tand the function with rescale it to whatever the player's resolution is. The rescaleToScreenAspectdetermines if the shape of the widget should should change with the aspect ratio of the screen (whether what was a square at 4:3 should become more of a rectangle at 16:9).

Am I wrong in thinking I don't need any calcTextSize? The vaiability of one-two characters between different resolutions might not be that big? (famous last words?)

I think you can skip it, but I'd leave more room than 2 digits. 4 just in case?

[Web-eWorks]

@impaktor

Am I wrong in thinking I don't need any calcTextSize? The vaiability of one-two characters between different resolutions might not be that big? (famous last words?)

You are in fact correct; this one's my bad. For some reason, even after reading the source, I was thinking that ui.sameLine took a relative offset instead of an absolute one. There's no need for ui.calcTextSize at all, and from what you're showing this works perfectly fine.

It looks like you've left room for three digits of offenses; although you might want to add just a little more space, it's totally optional and we will adjust it later if it ever becomes necessary.

[impaktor]

OK, pushed final(?) changes, basicaly settled to what we said above, just use ui.sameLine(55).

I also couldn't remove adding a small fix as a second commit, ping @vakhoir (or should I remove it?)

If anyone cares, you might want to check my indentation style, for breaking long lines.

[Web-eWorks]

@impaktor these changes look good! I can't pull them locally to test today, but if @vakhoir can sign off on it, I'm good to pull the merge trigger!

[vakhoir]

Looks good, but I'd make use of the rescaleUI function we talked about. If you have the sizes hardcoded, they'll be the same for other resolutins. See for example:

This was taken at 640x480. It doesn't look bad here, but you can see it does not look how you set it up at 1600x900.

[vakhoir]

I'd recommend to move this to widgetSizes as something like crimeRecordColumnWidth = 55. Then you can use this here as

Suggested change

	ui.sameLine(55)
	ui.sameLine(widgetSizes.crimeRecordColumnWidth)

Since the whole widgetSizes table is passed to rescaleUI, this will be automatically rescaled to the screen resolution.

[vakhoir]

Same here. I'd move it to widgetSizes.

@Web-eWorks, I've been wondering about something here. Does calling Vector2() from LUA call a constructor on the C++ side? If so do you reckon it would improve performance to just set a vector once on import and refer to it during rendering, rather then constructing it every frame? Or are any gains to small for it to matter?

[Web-eWorks]

Yes and yes. Ideally, call local v = Vector2() as few times as possible, and when you're updating the vector call v(x, y) instead of v = Vector2(x, y) to set the values without allocating a new vector object.

Overall gains will be incredibly tiny, considering that the entire pigui call tree runs in 1-2ms or so, but it is "best practice" for whatever that's worth.

[vakhoir]

and when you're updating the vector call v(x, y) instead of v = Vector2(x, y)

Oh, I didn't realize that was an option, sweet!

[vakhoir]

Same here

[vakhoir]

And same here

[impaktor]

@vakhoir I've fixed in accordance to your suggestions

I've pushed it as a separate commit, so you can check that it looks OK.

I'll squash it and rebase when you give thumbs up, so this can be merged afterwards.

Thanks for suggestions

[vakhoir]

Looks good!