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

Improved reload for solar body #210

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Improved reload for solar body #210

merged 3 commits into from
Jul 21, 2023

Conversation

Fueredoriku
Copy link
Contributor

The solar body now actually considers where the sun is, and if there are any obscuring objects to block the sun, making for more intuitive reload mechanics

solarBetterReload

@Fueredoriku Fueredoriku self-assigned this Jun 29, 2023
@Fueredoriku Fueredoriku changed the base branch from main to dev June 29, 2023 17:39
@Fueredoriku Fueredoriku requested a review from toberge June 29, 2023 17:53
Copy link
Contributor

@toberge toberge left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a few comments regarding the code, and I haven't tested it yet but the changes don't look too significant.

@Fueredoriku Fueredoriku requested a review from toberge July 18, 2023 19:00
@toberge
Copy link
Contributor

toberge commented Jul 19, 2023

Names look good but I'd still suggest writing transform.up and thest this before I approve

Copy link
Contributor

@toberge toberge left a comment

Choose a reason for hiding this comment

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

So in addition to that one comment I have reopened: After briefly testing the solar body, I do wish we could lerp between the two states because the transition from animated grid to pure darkness feels quite jarring. Other than that it works great, now it's actually usable, while not entirely OP since other players can bait the solar wielder into dark areas.

@Fueredoriku
Copy link
Contributor Author

I agree that lerp-ing to reload state is nice, but after testing I've concluded that lerping to the inactive state from active removes visual clarity in terms of showing new users what conditions turns the reload off. I kept the sudden change for gameplay concerns in that specific case and added lerp for the other :)

@Fueredoriku Fueredoriku requested a review from toberge July 21, 2023 16:56
Copy link
Contributor

@toberge toberge left a comment

Choose a reason for hiding this comment

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

it's probably fine now, you fixed things

@Fueredoriku Fueredoriku merged commit 0fde0c0 into dev Jul 21, 2023
@Fueredoriku Fueredoriku deleted the feature/solar-improved branch July 21, 2023 17:21
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.

2 participants