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

Donate to Cranks - incorporate info from factions #5445

Merged
merged 11 commits into from
Dec 21, 2022

Conversation

zonkmachine
Copy link
Member

Improving immersion in 'Donate to Cranks' by adding some faction info as string variables (faction name and the names of the police and military forces) and some new flavours to make use of them.
I tried to make the string interpolation in just one place but failed at that so it's now done in onCreateBB and onGameStart separately.

crank1

@Web-eWorks
Copy link
Member

The easiest way to handle the interpolation is to store the stringVariables table in the ad object in onCreateBB and do the interpolation using ad.stringVariables in both locations rather than reconstructing the stringVariables table. It's also best to avoid storing the final interpolated text in the ad object, as that's serialized to disk and increases the size of the savefile. (Technically this also applies to the stringVariables object, if you want to go the extra mile and reconstruct it each time the advert is placed.)

I'll need to do a further pass suggesting corrections to the grammar / structure of the ad titles later this week but overall I like the idea of this PR!

@zonkmachine
Copy link
Member Author

I'll need to do a further pass suggesting corrections to the grammar / structure of the ad titles later this week but overall I like the idea of this PR!

Yes. I'm not an author, that's for sure. ;P

The easiest way to handle the interpolation is to store the stringVariables table in the ad object in onCreateBB and do the interpolation using ad.stringVariables in both locations rather than reconstructing the stringVariables table.

OK. I think I managed to come up with something that works. I also added a {SYSTEM} variable.

Screenshot at 2022-12-18 23-41-44

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 left suggestions with various typo fixes, you should be able to go through and accept them all + commit the changes from the web interface if you have no objections.

Other than the grammar improvements, I think this PR is good to land!

data/lang/module-donatetocranks/en.json Outdated Show resolved Hide resolved
data/lang/module-donatetocranks/en.json Outdated Show resolved Hide resolved
data/lang/module-donatetocranks/en.json Outdated Show resolved Hide resolved
data/lang/module-donatetocranks/en.json Outdated Show resolved Hide resolved
data/lang/module-donatetocranks/en.json Outdated Show resolved Hide resolved
@zonkmachine
Copy link
Member Author

I've left suggestions with various typo fixes,

Ouch! That was worse than I expected.

Other than the grammar improvements, I think this PR is good to land!

OK. I've been looking into more stuff in 'Donate to Cranks' but I think I'm good for now. One caveat! I haven't actually tested this in other systems than the two available from the start menu. For instance, I don't know if families in the {FACTION}... works in. I'm looking into ways to hack myself to other stations. I have no intention to actually learn how to fly those things...
In any case, I think this should be tested more before merging.

@zonkmachine
Copy link
Member Author

dedicated their lives to keeping {SYSTEM} safe need your support!

{SYSTEM} is the only variable I'm worried about. I don't know if Game.system.name is useful always.

@Web-eWorks
Copy link
Member

Game.system.name will always be the translated string that's shown on the map as the system name. Given that it can be randomly generated quite often, there's very little further processing that can be done. The only thing you have to be wary of is Game.system is a nil value while in hyperspace.

@zonkmachine
Copy link
Member Author

OK. So just merge and see if it works for our explorers?

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.

Yes, at this point if you've seen no issues after testing locally, I'm fine to merge this and sort out any further bugs if they arise.

@zonkmachine
Copy link
Member Author

It's a go!

@Web-eWorks Web-eWorks merged commit d400f7d into pioneerspacesim:master Dec 21, 2022
@zonkmachine zonkmachine deleted the cranks branch December 21, 2022 22:46
@zonkmachine
Copy link
Member Author

@Web-eWorks Right, it's much cleaner to use the github builtin squash merge option. I always assume it because my PR tend to start out clean and then deteriorate exponentially. (for the record. If it's me, you don't have to ask to squash a PR).

@Web-eWorks
Copy link
Member

Y'know, I didn't even bother to look at the git history before merge... too much on the brain. It's not too bad, just some extra commits in the history.

@impaktor
Copy link
Member

Ideally, these commits should have been squashed.

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

Successfully merging this pull request may close these issues.

3 participants