-
-
Notifications
You must be signed in to change notification settings - Fork 565
Membrane graphics additions #6286
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
base: master
Are you sure you want to change the base?
Conversation
|
I have tested the visuals and I think it looks nice, but I have some comments:
|
|
Ah you didn't have to review yet, I'm still debating whether to actually include any changes other than membrane turning. But thanks for review |
|
I know, I just wanted to check out of curiosity :D |
I think that this is a pretty big concern as even now we have complaints about the current membrane graphics being too similar (ever since the 3D membranes implementation). So going even more in the direction of different membranes looking the same has to be avoided. |
|
It should work now. The bug where Luca species preview is still with the old membrane is still there though. I found that changing the number to something else at https://github.com/Revolutionary-Games/Thrive/blob/master/src/general/Species.cs#L336 can fix that but I don't know if it's the right place to change something |
|
That's not the right place. There should be a constant in the Constants file that controls a base hash for cells (it's next to a base value for cell layout views). |
|
Should do now, but visibility is still kinda poor in the preview anyways. Main stuff can be tested though |
…e.git into membrane_graphics_revamp
|
Told you this a bit ago, but wanted to write here that I love this change. |
|
I don't know if it looks good enough though |
| metadata={ | ||
| "imported_formats": ["s3tc_bptc", "etc2_astc"], | ||
| "vram_texture": true | ||
| "vram_texture": 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.
I think it is wrong to change this to not be a vram texture... Why was this change done?
| VERTEX.z += sin(worldVertex.x * movementWigglyNess - TIME / 4.0f) / 10.f | ||
| * wigglyNess * size; | ||
|
|
||
| // Bend when turning |
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.
Mixed tabs into the file here.
| render_mode unshaded; | ||
|
|
||
| // Any changes to these two must be matched in Membrane.gdshader | ||
| // Any changes to these two must be matc hed in Membrane.gdshader |
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.
| // Any changes to these two must be matc hed in Membrane.gdshader | |
| // Any changes to these two must be matched in Membrane.gdshader |
| { | ||
| ref var cellProperties = ref entity.Get<CellProperties>(); | ||
|
|
||
| if (cellProperties.CreatedMembrane != null) |
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 don't think it should be the visual creation system that applies turning property to the visuals, it should be a different system.
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 that if we want this effect in the game, then the technical implementation side will need some improvements.

Brief Description of What This PR Does
The bending-on-turn effect and possible internal decoration mesh
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.