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

chore: overhaul the block damage system #93

Merged
merged 7 commits into from
Aug 14, 2021
Merged

Conversation

skaldarnar
Copy link
Contributor

@skaldarnar skaldarnar commented Jun 24, 2021

  • doc: improve docstrings of BlockDamageRenderer
  • chore: empty default implementation for RenderSystem interface
  • doc: add better docstrings for BlockDamageRenderer
  • feat: broadcast health changed events (was owner events)

Requires MovingBlocks/Terasology#4789

@skaldarnar skaldarnar added this to In progress in v2.0.0 - Overhaul via automation Jun 24, 2021
@skaldarnar
Copy link
Contributor Author

skaldarnar commented Jun 24, 2021

Open issues

  • OnDamaged is an @OwnerEvent and thus only visible on the authority and the client owning the entity. Therefore, no client receives the event and thus can't react on it to show any particle effects.

    We may make the event a @BroadcastEvent, but I don't know the implications of that. There is an open card to understand networks events better.

    Solved by 8c234fd

  • On a client-only system, some default particle effects (with smoke or dust texture) are shown in addition to the expected square particles derived from the block texture. I have no idea where they are coming from 👀
    Edit: I don't think this is related to this PR, as I've seen it also without these changes.

@skaldarnar skaldarnar changed the title overhaul/damage systems chore: overhaul the block damage system Jun 24, 2021
@skaldarnar skaldarnar added Category: Doc Requests, Issues and Changes targeting javadoc and module documentation Multiplayer Affects aspects not visible in Singleplayer mode only Status: Needs Testing Requires to be tested in-game Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Chore Request for or implementation of maintenance changes labels Jun 24, 2021
@skaldarnar
Copy link
Contributor Author

skaldarnar commented Jul 11, 2021

I'm not really sure about the change from @OwnerEvent to @BroadcastEvent I did here. Some thoughts on this:

  • The server knows about the damaged entity anyways (regardless of OwnerEvent or BroadcastEvent event
  • The owner is currently the only system reacting to this client-side, so OwnerEvent would be sufficient
  • Should other clients know about whether an entity was actually damaged or not? There might be game modes that want to hide this from opponent players, but sending this event would make it easy to add a client-side modification to visualize a damage effect.
  • Vice versa, not sending this event to other clients prevents them from producing additional client-side effects.

I guess, that for now it's better to leave this as @OwnerEvent and only broaden the scope if we have a concrete use case for this.

Edit: Reverted in 9781124

@skaldarnar skaldarnar marked this pull request as ready for review July 11, 2021 11:24
@skaldarnar
Copy link
Contributor Author

I can run the MTE tests just fine locally 🤔

image

@jdrueckert jdrueckert merged commit a4b91d2 into develop Aug 14, 2021
v2.0.0 - Overhaul automation moved this from In progress to Done Aug 14, 2021
@jdrueckert jdrueckert deleted the overhaul/damage-systems branch August 14, 2021 18:49
@jdrueckert
Copy link
Member

Module Overhaul - Health

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Doc Requests, Issues and Changes targeting javadoc and module documentation Multiplayer Affects aspects not visible in Singleplayer mode only Status: Needs Testing Requires to be tested in-game Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Chore Request for or implementation of maintenance changes
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants