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

Handle research trigger 'craft-item' #327

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

veger
Copy link
Collaborator

@veger veger commented Oct 23, 2024

While checking for the Space Age changes of #320, I noticed 'new' errors when loading a project. Which was triggered by the (lack of) support of the new research triggers.

Adding the craft-item type was fairly easy (I was fiddling a bit and it worked):
<removed outdated screenshot, see comments for newer one>

So I cleaned the code a little and made this PR.

Note that supporting mine-entity was also easy, but for some reason it makes most of the items inaccessible... So I did not include it in this PR. We need to take a better look.
The other types, I did not try as their implementation was not directly apparent to me (and craft-fluid is not in use, so I could not try/test)

@veger veger requested a review from shpaass as a code owner October 23, 2024 22:40
@veger veger requested a review from SWeini October 23, 2024 22:41
@SWeini
Copy link
Collaborator

SWeini commented Oct 24, 2024

Thanks for doing the UI work.

I don't have anything against this change, but I want to explore the topic of trigger techs a bit further:

For me the big challenge was to include the triggers into the tech tree / milestone analysis. Unfortunately it doesn't build up one big graph and then "simply" walks along edges and marks nodes as "available". It has some really weird logic. So I went the easy way, basically enabled all techs (by binding SpecialNames.TechnologyTrigger to the character) and hoped that the rest of the tech tree simply contains enough information for everything else to work. For vanilla that was OK, but the mods we need a better long-term solution.

e.g. the MineEntityTechnologyTrigger trigger needs a lot of requirements:

  • if entity is non-resources entity (I guess this works, but I haven't checked): character needs access to a surface/planet where that entity can auto-spawn, or player is able to build one itself
  • if entity is resource:
    • character needs access to a surface/planet where that resource can auto-spawn
    • character can mine its category or has access to a mining drill that can, and access to fuel for that drill, and the drill is allowed on that same surface/planet where the resource is
    • we need to respect MiningWithFluidModifier, which we don't support at all yet

To sum it up, I'm not surprised that the mine-entity trigger didn't work for you out of the box - and we need to think about how much brainpower we put into the trigger techs and how we prioritize it.

@SWeini
Copy link
Collaborator

SWeini commented Oct 24, 2024

is it possible to also remove the ingredients that show up the "normal way"

image

@veger veger force-pushed the research-trigger branch 2 times, most recently from 6a87300 to e398d37 Compare October 24, 2024 07:24
@veger
Copy link
Collaborator Author

veger commented Oct 24, 2024

I removed the 'recipe part' of the card when showing a research trigger technology, as all of the recipe details did not make much sense for the them:
image

@veger
Copy link
Collaborator Author

veger commented Oct 24, 2024

@SWeini we could add another field to the Technology class instead of (ab)using the RecipeOrTechnology.ingredients field... This might prevent unforeseen/future issues...? 🤔

and improve error reporting a little for unsupported research triggers
@shpaass
Copy link
Owner

shpaass commented Oct 29, 2024

Rebased on fresh master

@shpaass shpaass merged commit de43a51 into shpaass:master Oct 29, 2024
1 check passed
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