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

feat(code): Custom outfit/ship prices and sell type #122

Open
wants to merge 36 commits into
base: experimental
Choose a base branch
from

Conversation

TheGiraffe3
Copy link

@TheGiraffe3 TheGiraffe3 commented Nov 2, 2024

Enhancement

Summary

A re-PR to Delta of endless-sky#6404.

See the original PR for more details.

Examples

pricing outfits "remnant broken jump drives"
  import
  location
    government "Remnant"
  conditions
    has "Remnant Bounty Hunter: completed"
  
  value
    "Jump Drive (Broken)" 1000000
pricing outfits "remnant broken jump drives"
  import
  location
    government "Remnant"
  conditions
    has "Remnant Bounty Hunter: completed"
  
  offset
    "Jump Drive (Broken)" 0.1%

Testing Done

None so far, though the original PR was tested throughly.

This is a squash-merge to a Delta branch of the pull request endless-sky#6404

Co-authored-by: Hurleveur <[email protected]>
Co-authored-by: Hurleveur <[email protected]>
Co-authored-by: Ben Hauch <[email protected]>
Co-authored-by: tibetiroka <[email protected]>
Co-authored-by: warp-core <[email protected]>
Co-authored-by: Amazinite <[email protected]>
Co-authored-by: EjoThims <[email protected]>
@Zitchas
Copy link
Member

Zitchas commented Nov 2, 2024

Awesome! Thanks for taking this on!

I'm not sure what the problem is at the moment, and there isn't anything by GitHub actions in the files right now.
Checks should pass now.
@TheGiraffe3
Copy link
Author

TheGiraffe3 commented Nov 2, 2024

Looks like ship support may be harder than I thought it would be.
TheGiraffe3#31

Checks are failing on one problem and don't seem to be giving an error message.
@TheGiraffe3 TheGiraffe3 changed the title feat(code): Custom outfit prices and sell type feat(code): Custom outfit/ship prices and sell type Nov 4, 2024
@TheGiraffe3 TheGiraffe3 marked this pull request as ready for review November 4, 2024 10:54
@TheGiraffe3
Copy link
Author

Does anybody have ideas on how to fix the failing checks?
It appears that they don't like CustomOutfitSaleManager::customOutfitSales but I'm pretty sure I defined it correctly, and I'm not sure how to redefine it.

@Zitchas
Copy link
Member

Zitchas commented Nov 4, 2024

I'm going to take a look at it, but my coding skill is weak. Might need to get a better coder to take a look at offer advice.

@Zitchas
Copy link
Member

Zitchas commented Nov 4, 2024

OK, Visual Studio provides a bit more detail:
error : undefined symbol: private: static class std::map<enum CustomOutfitSale::SellType, class CustomOutfitSale, struct std::less<enum CustomOutfitSale::SellType>, class std::allocator<struct std::pair<enum CustomOutfitSale::SellType const, class CustomOutfitSale>>> CustomOutfitSaleManager::customOutfitSales

And a list of places that it is referenced by (CustomSaleManager 38, 147, 53) (referenced 5 more times beyond that, but it only lists those)

Followed by the similar error:

error : undefined symbol: private: static class std::map<enum CustomShipSale::SellType, class CustomShipSale, struct std::less<enum CustomShipSale::SellType>, class std::allocator<struct std::pair<enum CustomShipSale::SellType const, class CustomShipSale>>> CustomShipSaleManager::customShipSales

in CustomSaleManager.cpp L65, 154, 80; and referenced 5 more times.

I hope this is helpful info, anyway.

Doing a search for the error message turned up this stackoverflow article:
https://stackoverflow.com/questions/20439003/clang-linking-error-for-c-static-class-undefined-symbols

@TheGiraffe3
Copy link
Author

TheGiraffe3 commented Nov 4, 2024

That gave me a really random idea, so we'll see if it's that.
I think maybe the problem is that functions were private and not accessible by others.

@Zitchas
Copy link
Member

Zitchas commented Nov 4, 2024

@Hurleveur I know you're busy and can't do a deep dive on code or anything, but if you could take a look and offer a few pointers or insight sometime, it'd be much appreciated.

edit: TheGiraffe3: Don't hold up your work waiting for Hurleveur, as there's really no timeline as to when they may be able to provide some advice. It may be weeks or months. But they're probably the most familiar with how it works, so hopefully someday they'll be able to comment.

edit: I just downloaded your recent changes and ran it through VS's build process, and it's turning up the exact same errors as before.

I'm not sure why, but something in my head is nagging me thinking that those things being private is fine, but there needs to be something public but internal to those sections that calls them and makes their stuff available to be called.

@Hurleveur
Copy link
Member

There's not really an easy way to fix it, I could explain but someone would need a good setup to use for debugging along with understanding of what's going on in order to resolve it.
It could be broken due to smth else changing in the game or bad conflict management on my part.
The different integration tests allow knowing what exactly is broken but figuring out how requires knowing how they interact with one another.

@Zitchas
Copy link
Member

Zitchas commented Nov 4, 2024

I think for the moment, I'm more concerned about the build failures than the integration tests. Both are important, I know; but picking a place to start, the build failures seems the logical place.

@warp-core
Copy link

warp-core commented Nov 4, 2024

static class fields need to be initialised outside of the class definition. Or, they can be made inline and initialised inside the class definition.

@warp-core
Copy link

warp-core commented Nov 4, 2024

You only need to make CustomOutfitSaleManager:customOutfitSales and CustomShipSaleManager::customShipSales inline, not any of the methods.

@xobes
Copy link

xobes commented Nov 7, 2024

The different integration tests allow knowing what exactly is broken but figuring out how requires knowing how they interact with one another.

I want to learn more about the tests, didn't see anything in the wiki.

Also, per #15:

Instead of creating a new comment if you want to reply, write a reply on the comment instead just to keep things nice and tidy.
... how?

@warp-core
Copy link

warp-core commented Nov 7, 2024

Also, per #15:

Instead of creating a new comment if you want to reply, write a reply on the comment instead just to keep things nice and tidy.

... how?

The comments on a GitHub Discussion are arranged into two dimensions.
The first being the "top level comments". You create a new one of these with the text box at the very bottom of the page, similar to comments on an Issue or Pull Request.

image

Unique to Discussions is that each of these comments can also have a chain of comments following it. You can add to one of these with the smaller text box at the bottom of each group of coments.

image

The request you have quoted is for comments following on from an existing comment to use the chain nested under that comment, instead of creating a new "top level" comment.

More information from GitHub htemselves: https://docs.github.com/en/discussions/collaborating-with-your-community-using-discussions/participating-in-a-discussion

@TheGiraffe3
Copy link
Author

Alrighty. If this is up and working, then the next step will be testing it.

A few good examples:

  • Having a selection of advanced ships and weaponry in the pirate sales list sell for significantly more than their typical list price.
  • Have one or two of the cheapest ships in the pirate sale list (shuttles, maybe?) sell for somewhat below normal price (say, about 75% or so)
  • Have the Remnant outfitter buy Hai quantum keystones for something close to the price of Remnant Key Stones but without making them visible in the outfitter.

Added all those, plus the Remnant and UHai buying JD for more.
Any other thoughts?

@TheGiraffe3
Copy link
Author

TheGiraffe3 commented Nov 12, 2024

I'm thinking perhaps the Wanderers wouldn't buy Pug technology (or rather, would pay very little for it).
Maybe same with the Hai/humans for the Quarg, and pirates will pay more for the forbidden Quarg technology?

That could get a little labor-intensive though.

@Zitchas
Copy link
Member

Zitchas commented Nov 12, 2024

I'm not sure about the Wanderer/Pug one. It'd be good to get input from those teams on that.

But definitely, human space by and large shouldn't pay much for Quarg tech. (I suspect that some specific groups, like pirates, within that who certainly would pay good money for it, but most would probably keep it very quiet; so just having the pirates as the only place to really be able to sell the stuff at a good price for now)

Don't feel the need to do a full pass or anything on this. Just having a few items so that we can test a selection of different situations (outfit being sold for more somewhere, outfit being sold for cheaper somewhere, a ship or two similar example.

The bulk of the changes I would expect to happen in later PRs.

@TheGiraffe3
Copy link
Author

I'm not sure about the Wanderer/Pug one. It'd be good to get input from those teams on that.

Just throwing the thought out there. Not sure about it either.

But definitely, human space by and large shouldn't pay much for Quarg tech. (I suspect that some specific groups, like pirates, within that who certainly would pay good money for it, but most would probably keep it very quiet; so just having the pirates as the only place to really be able to sell the stuff at a good price for now)

Probably the Unfettered as well.
I've also added the Hai as a bad place to sell technology - it's quite easy, and makes sense.

@Zitchas
Copy link
Member

Zitchas commented Nov 12, 2024

They generally don't come across as super interested in acquiring alien technology, yeah.

Remnant are the opposite of that. Aside from the Pug, they are quite interested in acquiring alien tech and would generally pay more for most of it.

@TheGiraffe3
Copy link
Author

Remnant are the opposite of that. Aside from the Pug, they are quite interested in acquiring alien tech and would generally pay more for most of it.

This could run into balance problems where people buy a whole bunch of technology, cart it across the galaxy, and sell it for more than it was bought for, so it would probably have to be a smaller price increase (like 5%).
I'm not super interested in compiling a list of every alien outfit by hand, though.
I noticed that in the Command Line Arguments wiki page, you said it was possible to tell ES to print a list of every outfit. Does that put them on separate lines with quotation marks? And if so, could somebody run the command and send me the output over GitHub?
Those command line arguments don't seem to work on MacOS.

@Zitchas
Copy link
Member

Zitchas commented Nov 12, 2024

It outputs the data as a CSV file, which is trivial for any spreadsheet program to open. I can do it, but it may not be until tomorrow.

@xobes
Copy link

xobes commented Nov 13, 2024

This could run into balance problems where people buy a whole bunch of technology, cart it across the galaxy, and sell it for more than it was bought for, so it would probably have to be a smaller price increase (like 5%).

Is this really any different than commodities? It would make sense for Human space to fetch a higher price on higher tech, and vice-versa -- Human tech worth less in higher-tech space.

... to that end, wouldn't tech pricing be something that is more variable like commodities, as well? Rather than fixed/hard-coded pricing locked in?

Calculating depreciation will be fun. But then again, there's already a multiplier at work (100% down to 25%) that could be leveraged -- shops could sell at 125% or 90% (custom tech multipliers) to cost. Actually that multiplier would work the whole way down the chain.

System A = 125% for Tech X
System A = 90% for Tech Y

System B = 105% for Tech X
System B = 110% for Tech Y

Could also be somewhat dynamic -- fluctuating (like commodities do)

@Zitchas
Copy link
Member

Zitchas commented Nov 13, 2024

Here you go:

Outfits:
ESD-outfits - Copy.csv

Outfit with sales: At a glance, it looks like it lists each outfit, and then next to it lists every outfitter that includes it (this probably does not take events and missions into account)
ESD-outfits-sales - Copy.csv

@TheGiraffe3
Copy link
Author

This could run into balance problems where people buy a whole bunch of technology, cart it across the galaxy, and sell it for more than it was bought for, so it would probably have to be a smaller price increase (like 5%).

Is this really any different than commodities? It would make sense for Human space to fetch a higher price on higher tech, and vice-versa -- Human tech worth less in higher-tech space.

... to that end, wouldn't tech pricing be something that is more variable like commodities, as well? Rather than fixed/hard-coded pricing locked in?

Calculating depreciation will be fun. But then again, there's already a multiplier at work (100% down to 25%) that could be leveraged -- shops could sell at 125% or 90% (custom tech multipliers) to cost. Actually that multiplier would work the whole way down the chain.

System A = 125% for Tech X System A = 90% for Tech Y

System B = 105% for Tech X System B = 110% for Tech Y

Could also be somewhat dynamic -- fluctuating (like commodities do)

I've got a re-PR of an orphaned pull request to upstream: endless-sky#10676
I am having trouble resolving a comment there but that's not really related to this.

@TheGiraffe3
Copy link
Author

Well, apparently ships don't work, even though they've got the exact same code as outfits.

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

Successfully merging this pull request may close these issues.

5 participants