-
Notifications
You must be signed in to change notification settings - Fork 999
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
Fining Revamp - getting scanned with illegal cargo gets you a new hail panel + more #6376
base: master
Are you sure you want to change the base?
Conversation
Quick thought(s)- I'm very excited for government-specific legalities getting into the game ASAP, it would open up lots of opportunities for strict regions or apathy in law. |
You're only surrendering your illegal cargo. I don't really see a reason for the player to choose to surrender everything though. |
shrug Would it be possible to, when confronted by authorities, give the player the option to refuse surrender, or surrender all their illegal cargo, or surrender only a portion of their illegal cargo? A way to still make a buck, if the coppers don't realize you aren't being honest. There's a chance they didn't pick up some of that cargo, and betraying your whole hand is costly. It might be worth it to risk hostilities for a little extra profit. A game of limited information. |
It's already possible to refuse to surrender, in which case you provoke the gov and they start attacking you. For the other request, idk maybe. |
source/Government.cpp
Outdated
@@ -116,6 +118,8 @@ void Government::Load(const DataNode &node) | |||
bribe = child.Value(1); | |||
else if(child.Token(0) == "fine" && child.Size() >= 2) | |||
fine = child.Value(1); | |||
else if(child.Token(0) == "bribe factor" && child.Size() >= 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect to also see a "reputation hit"
factor when the player flees. (Not a large factor for most governments, but fleeing should have a little bit longer-term effects.)
For this PR I would suggest to just focus on surrendering all illegal cargo and to keep those ideas for a possible follow-up PR. (I would assume the authorities in most cases to board the offending ship and take the illegal cargo directly btw). |
"Waiting on OP" due to the open merge-conflicts. |
I prefer an all or nothing approach when it comes to surrendering cargo. It keeps things simple and straightforward. |
A related PR contains a nice comment about allow fines to both have a "once once discovered" and a "per item when discovered" fine part. I feel that such an approach would also be nice here. |
@quyykk Please address the conflicts and outstanding reviews on this PR, or we'll have to close it. |
Co-authored-by: tibetiroka <68112292+tibetiroka@users.noreply.github.com>
The illegal tests need to be changed a bit to account for the new panel, idk what changes happened to it. edit: I think that changing line 321 of tests_illegal_atrocity.txt should do it. Not sure what should happen, but previously clicking done was all, maybe now you need to input "escape" instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked at the parsing for now
@@ -602,15 +601,15 @@ int CargoHold::IllegalCargoFine(const Government *government) const | |||
return -1; | |||
if(fine < 0) | |||
return fine; | |||
totalFine = max(totalFine, fine / 2); | |||
totalFine += (fine / 2) * it.second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to make the fines stack but maybe use a sqrt or smth?
Or maybe we should just rebalancr fines afterwards, but 450M caus of nerve gas is going to hurt (which is good), possibly too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah definitely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot about this?
Phone github... I didnt click aprove xD |
Update warp-core changes for HailPanel in PlayerHailPanel (cpp and h) for endless-sky#8360 Updated changes from endless-sky#8236 across moved code for `Format::CreditString`. - IllegalHailPanel.cpp - PlayerHailPanel.cpp
0c3aafd
to
ccfaefa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things to consider
I hope this can be the beginning of a more dynamic hail system for npcs where they can eventually give you missions and the like
anyway I didnt look at all of it but this should give you some work already, code looks good and I'll be testing this at some point to make sure all's working fine
@@ -602,15 +601,15 @@ int CargoHold::IllegalCargoFine(const Government *government) const | |||
return -1; | |||
if(fine < 0) | |||
return fine; | |||
totalFine = max(totalFine, fine / 2); | |||
totalFine += (fine / 2) * it.second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot about this?
bribeFactor = add ? bribeFactor + child.Value(valueIndex) : child.Value(valueIndex); | ||
else if(key == "interdiction" || key == "interdiction bribe") | ||
{ | ||
const bool isInterdiction = key == "interdiction"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt we check this earlier so we use it in the if just above too? mb we can even set it in the if idk
string &text = isInterdiction ? interdiction : interdictionBribe; | ||
bool &seen = isInterdiction ? hasInterdiction : hasInterdictionBribe; | ||
|
||
if(!seen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comment for what seen is?
isnt this just for a reset of it when redefined - in which case it should be above? idk I'm a bit confused by this
|
||
static const std::string defaultMessage | ||
= "You've been detected carrying illegal <type> and have been issued a fine of <fine>." | ||
" \n\tDump your cargo immediately or we'll be forced to disable and board your ship."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" \n\tDump your cargo immediately or we'll be forced to disable and board your ship."; | |
" \n\tDump your cargo immediately or we'll be forced to open fire."; |
seems better as a default to me?
// is automatically removed since the missions are failed. | ||
for(const auto &pair : scannedShip.Cargo().Outfits()) | ||
if(pair.first->Get("illegal") || pair.first->Get("atrocity") > 0.) | ||
scannedShip.Jettison(pair.first, pair.second, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
say you have illegal cargo, but also atrocities, but they've only discovered one
should you be forced to dump all of it?
or is it always discover all/nothing?
|
||
player.Accounts().AddCredits(-bribe); | ||
Messages::Add("You bribed a " + hailingShip.GetGovernment()->GetName() + " ship " | ||
+ Format::CreditString(bribe) + " to avoid paying a fine today." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just find it weird you can bribe them to avoid the fine lol, one would just take the lowest cost option without much consequences? also more generic caus it could be to ignore the atrocity (or not?)
+ Format::CreditString(bribe) + " to avoid paying a fine today." | |
+ Format::CreditString(bribe) + " to leave you alone." |
else | ||
header = ship->DisplayModelName() + " (" + gov->GetName() + "): "; | ||
// Drones are always unpiloted, so they never respond to hails. | ||
bool isMute = ship->GetPersonality().IsMute() || (ship->Attributes().Category() == "Drone"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now, ik you didnt write this but I think we should change it eventually given the new CanSendHail method and the possibility for aliens with unknown languages to say smth more interesting than you dont recognize stuff
else if(gov->IsEnemy()) | ||
{ | ||
// Enemy ships always show hostile messages. | ||
// They either show bribing messages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's on me ig but that's not the case anymore they can show normal msgs now instead of the bribing all the time
|
||
// Do nothing if there is nothing to fine for or the target ship has already been fined today. | ||
if(!punishment.HasPunishment() || fined.count(ship.GetGovernment())) | ||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt we have a const static empty for this?
Feature Details
This PR introduces the following features:
"bribe factor"
)UI Screenshots
Usage Examples
Testing Done
Tested getting fined in space with various illegal outfits and cargo.
Performance Impact
Negligible.