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

Add Winter holiday event models to the lobby #768

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

RoyalBlue1
Copy link
Contributor

@RoyalBlue1 RoyalBlue1 commented Dec 3, 2023

Models will be shown from 18.12. to the 06.01.
image
image

the tree model is from r5 and modified by 4Volt
The other models are from 4Volt

Co-authored-by: @EM4Volts

Tree model from r5 and modified by 4Volt
other models are from 4Volt
Co-authored-by: EM4Volts <[email protected]>
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Changes mostly look good, just some incorrect formatting.

I'm not a fan of the multiple rpaks approach, but this has been discussed on discord (4v doesn't want to use the RePak refactor until it has a github release)

@ASpoonPlaysGames ASpoonPlaysGames added the needs testing Changes from the PR still need to be tested label Dec 4, 2023
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Apart from formatting, this looks good.
I could test this by modifying the date, and verify new models appear in the lobby.

image

image

Private lobby menu is however hard to read white-on-white, I don't know how this could be fixed (rotating the camera upwards maybe? Adding another present just below menu position?)

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

(bikesheddding)
A bunch of newly added files are missing trailing newlines

Northstar.Custom/paks/rpak.json Outdated Show resolved Hide resolved
@EM4Volts
Copy link
Contributor

EM4Volts commented Dec 7, 2023

Private lobby menu is however hard to read white-on-white, I don't know how this could be fixed (rotating the camera upwards maybe? Adding another present just below menu position?)

i think blue tried looking into moving cam or something. in the end we decided on sticking with this. since one menu is usable in other menus aswell, and the other is the store which isnt usable anyway. its a small time window and u can also still use the menu if u hover over it as the button label colors change then.

@catornot
Copy link
Member

catornot commented Dec 7, 2023

i think blue tried looking into moving cam or something. in the end we decided on sticking with this. since one menu is usable in other menus aswell, and the other is the store which isnt usable anyway. its a small time window and u can also still use the menu if u hover over it as the button label colors change then.

you should be able to edit the camera position with by moving menu_camera_target and menu_scene_ref.

@RoyalBlue1
Copy link
Contributor Author

RoyalBlue1 commented Dec 8, 2023

the camera is set through animations so we have 2 Options
1 use one of the other animations
2 modify the animation
both are not really that good of an option and without writing a whole new api changing it would do it permanently

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good enough, missing some localisation but this is a bit time sensitive so yeah

@EM4Volts
Copy link
Contributor

EM4Volts commented Dec 8, 2023

the camera is set through animations so we have 2 Options 1 use one of the other animations 2 modify the animation both are not really that good of an option and without writing a whole new api changing it would do it permanently

i dont think changing it would be bad. maybe we can sit together in the coming days and work out a solution for it.
currently the vanilla screen is just blank aswell so changing the camera to maybe face 180 degree wouldnt really change what stuff looks like

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good

@EM4Volts
Copy link
Contributor

EM4Volts commented Dec 13, 2023

"needs testing" just tested by changing the dates in script to trigger early. works.

@GeckoEidechse
Copy link
Member

missing some localisation

Missing localisations are easier to add post-merge actually as they are picked up by weblate which is way more accessible to non-developers than GitHub anyway ^^

@ASpoonPlaysGames
Copy link
Contributor

Missing localisations are easier to add post-merge actually as they are picked up by weblate which is way more accessible to non-developers than GitHub anyway ^^

Nah, because like its just using string literals, so it can't be localised atm.

We can still just fix this later very easily but yeah

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Requests from reviews have been addressed.

@GeckoEidechse GeckoEidechse added READY TO MERGE This mergeable right now and removed needs testing Changes from the PR still need to be tested labels Dec 14, 2023
@GeckoEidechse GeckoEidechse merged commit f4df314 into R2Northstar:main Dec 14, 2023
3 checks passed
@RoyalBlue1 RoyalBlue1 deleted the nsChristmasEvent2 branch January 8, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants