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

Review use of Exts.IsTraitEnabled #14445

Closed
RoosterDragon opened this Issue Nov 27, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@RoosterDragon
Member

RoosterDragon commented Nov 27, 2017

Exts.IsTraitEnabled is being used against a collection of TraitInfos rather than traits in some places. It only works against traits. See #14444 (comment).

We should review all usages and correct them.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 7, 2017

Member

Fixed some of it in #14477. I'll fix the tooltip stuff in #14444.

The one other non-tooltip related bad use I can see is

var size = TraitsImplementing<IAutoRenderSize>().Select(x => x.RenderSize(this)).FirstOrDefault(Exts.IsTraitEnabled);

This runs Exts.IsTraitEnabled on an int2 which is always true.

However the correct method is unclear here.

  • If we moved the check against the trait, what happens if there are no matches? Should it be First and throw? Should we use FirstOrDefault and assume no render bounds if there are no matches?
  • DetermineRenderBounds only runs once at actor creation. Therefore traits enabling/disabling themselves will be ignored - do we need to change this to refresh every tick? If so we have a much bigger change on our hands as a lot of consuming code relies on RenderBounds being invariant. It would also have to be modified to receive updates when the bounds change.
Member

RoosterDragon commented Dec 7, 2017

Fixed some of it in #14477. I'll fix the tooltip stuff in #14444.

The one other non-tooltip related bad use I can see is

var size = TraitsImplementing<IAutoRenderSize>().Select(x => x.RenderSize(this)).FirstOrDefault(Exts.IsTraitEnabled);

This runs Exts.IsTraitEnabled on an int2 which is always true.

However the correct method is unclear here.

  • If we moved the check against the trait, what happens if there are no matches? Should it be First and throw? Should we use FirstOrDefault and assume no render bounds if there are no matches?
  • DetermineRenderBounds only runs once at actor creation. Therefore traits enabling/disabling themselves will be ignored - do we need to change this to refresh every tick? If so we have a much bigger change on our hands as a lot of consuming code relies on RenderBounds being invariant. It would also have to be modified to receive updates when the bounds change.
@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 7, 2017

Member

@reaperrr - you refactored the bounds stuff recently - how is it intended to work?

Member

RoosterDragon commented Dec 7, 2017

@reaperrr - you refactored the bounds stuff recently - how is it intended to work?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 7, 2017

Contributor

@pchote has another refactor of the render bounds stuff in the pipeline, I'm sure he'll fix this as part of that.

Contributor

reaperrr commented Dec 7, 2017

@pchote has another refactor of the render bounds stuff in the pipeline, I'm sure he'll fix this as part of that.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 7, 2017

Member

Yes, DetermineRenderBounds, IAutoRenderSize, and a bunch of related code are about to disappear.

Member

pchote commented Dec 7, 2017

Yes, DetermineRenderBounds, IAutoRenderSize, and a bunch of related code are about to disappear.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 11, 2017

Member

And indeed is had done so with the merging of #14482. Looks like that's everything.

Member

RoosterDragon commented Dec 11, 2017

And indeed is had done so with the merging of #14482. Looks like that's everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment