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

Several minor fixes to ModelWidget #21117

Merged
merged 6 commits into from Oct 13, 2023
Merged

Conversation

abcdefg30
Copy link
Member

See commits. (I suggest reviewing them in isolation.) There were a few unnecessary checks and unused variables. Caching values which don't need any calculations does not make any sense if they are immediately overwritten on change anyway. Light pitch and yaw were also ignored previously.

@@ -190,9 +190,6 @@ public override void PrepareRenderables()
if (cameraAngle != cachedCameraAngle)
cachedCameraAngle = cameraAngle;

if (cachedVoxel == null)
return;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remove cachedVoxel in the 4th commit. Why is this separated to the first commit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can merge both together if you want, this was just the first change I made on this file, see the commit description. (This condition was always false.)

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR seems to have glitched shadows in the asset browser

In this PR

PR.mov

On bleed

Bleed.mov

@abcdefg30
Copy link
Member Author

Are you sure that this is wrong? On bleed we didn't use any light calculation at all. This is now using the same as ingame, so the light is coming at an angle. The thing is that the ground the shadow is projected on isn't rotated with the model in the UI.

@PunkPun
Copy link
Member

PunkPun commented Oct 13, 2023

IMO it's better to keep the original behaviour as it looks way better

@abcdefg30
Copy link
Member Author

Restored the light settings bleed (accidentally) used.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PunkPun PunkPun merged commit 72bb6c4 into OpenRA:bleed Oct 13, 2023
3 checks passed
@abcdefg30 abcdefg30 deleted the modelWidget branch October 13, 2023 12:30
@PunkPun
Copy link
Member

PunkPun commented Oct 13, 2023

Changelog

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.

None yet

3 participants