-
-
Notifications
You must be signed in to change notification settings - Fork 479
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
Add the ferroplast organelle, and remodel rusticyanin #5125
Add the ferroplast organelle, and remodel rusticyanin #5125
Conversation
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.
If I'm reading this correctly, the ferroplast is made up of multiple parts. This is not recommended as that requires the use of OrganelleMeshWithChildren
to work correctly. So I'd consider if it is possible to merge this into a single model (if not then this needs the same approach setup as other models that use OrganelleMeshWithChildren
at least the nucleus does this).
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 used a separate texture for the membrane, so I needed a separate model. I did also make the iron chunks a separate model, so they could move separately. I'll add OrganelleMeshWithChildren and set them to use the same shader.
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 guess that's fine, I just don't want there to be too many graphically intense organelles (as with 100+ cells on screen where each cell might have up to like 10 copies, it quickly adds up a bunch of extra drawcalls if there are organelles that require multiple drawcalls). The nucleus is a bit of a special case as you can have only one of them per cell. But I guess this is fine as there won't be that many new organelles added anymore.
"ChanceToCreate": 0.5, | ||
"Density": 1100, | ||
"DisplayScene": "res://assets/models/organelles/Ferroplast.tscn", | ||
"Name": "Ferroplast", |
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 think this line is where the CI failure comes from, the name here needs to be a translation key.
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.
Yes, it copied that value into all the translation files, and then caused errors due to the lower case.
"AtpProductionAbove": { | ||
"Atp": 25 | ||
}, |
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.
Why is there this kind of condition? What's the reasoning why ferroplast is unlocked when you have a big cell producing a bunch of ATP?
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 had that in there because the mitochondrion also used it. I didn't know if it was worth having there or not. Not sure why I increased the value...
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'd remove this unlock condition. Also with endosymbiosis merged, you should mark the ferroplast as the upgraded version of rusticyanin so that it becomes available through endosymbiosis.
@@ -21,7 +21,7 @@ compress/lossy_quality=0.7 | |||
compress/hdr_compression=1 | |||
compress/normal_map=0 | |||
compress/channel_pack=0 | |||
mipmaps/generate=true | |||
mipmaps/generate=false |
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.
This needs to be kept on for the icon to look good.
e4a8582
to
f8e4655
Compare
Was this closed on accident or why was this closed? |
I've been having a nightmare trying to update the branch. I'm going to redo it on a different branch. |
You don't need to close the PR to do that. Even if force pushing fails (which already can replace any and all commits in this PR), modifying the PR to use a different source branch is possible. |
Brief Description of What This PR Does
Adds in models, icon and settings for the ferroplast - a non-LAWK, eukaryotic organelle that is a more efficient equivalent of rusticyanin. Also remodels the rusticyanin to resemble the real-life protein.
Related Issues
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.