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

Light Effects for rides reintegrated with less lag and option to turn off. #10970

Merged
merged 20 commits into from
Mar 20, 2020
Merged

Light Effects for rides reintegrated with less lag and option to turn off. #10970

merged 20 commits into from
Mar 20, 2020

Conversation

WantDiscussion
Copy link
Contributor

Light effects now only adds lights for vehicles on screen. Only minor lag when many vehicles are on screen. Added option to turn on/off ride lights.

Before:
Annotation 2020-03-19 110021

After:
Annotation 2020-03-19 105916

@AaronVanGeffen
Copy link
Member

Thanks for working on this.

It's not necessary to close pull requests and open a new one whenever you make a change. GitHub will update your PR whenever you push.

@AaronVanGeffen AaronVanGeffen added the pending rebase PR needs to be rebased. label Mar 19, 2020
Copy link
Member

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Indentation looks odd, but the clang format succeeded. In any case, there are some builds failing, please take a look

src/openrct2-ui/windows/Options.cpp Show resolved Hide resolved
src/openrct2-ui/windows/Options.cpp Show resolved Hide resolved
src/openrct2/config/Config.cpp Show resolved Hide resolved
@tupaschoal
Copy link
Member

I also would appreciate gifs of before/after the change so we have a better idea of things in motion.

@WantDiscussion
Copy link
Contributor Author

I also would appreciate gifs of before/after the change so we have a better idea of things in motion.

https://imgur.com/a/uJMyMUQ

@duncanspumpkin
Copy link
Contributor

Is additional options required? Would anyone actually want the vehicle lights to be off?

@pizza2004
Copy link
Contributor

I don't think the additional option is necessary personally. Most people won't even play with the day/night cycle (especially given that it has nothing to do with the actual days in game), let alone the light effects. This seems like such a tiny preference one way or the other that if it doesn't work well for you as far as FPS goes you probably just shouldn't be using the light effects at all.

@WantDiscussion
Copy link
Contributor Author

Is additional options required? Would anyone actually want the vehicle lights to be off?

There is a bit of lag when using uncapped fps so I thought I'd add the option.

@Gymnasiast
Copy link
Member

Hm, I'm not sure why the vehicles should give off light. That's not typically what happens at night - vehicles stay dark.

@AaronVanGeffen
Copy link
Member

I think it helps see where vehicles are in the dark, and on top of that, it's very aesthetically pleasing.

@WantDiscussion
Copy link
Contributor Author

Hm, I'm not sure why the vehicles should give off light. That's not typically what happens at night - vehicles stay dark.

Well transports rides would have interior lights, and possibly the mini cars. But true the go karts and boats probably wouldn't.

@IntelOrca
Copy link
Contributor

If you look at the original PR for the lights, the steam trains gave off light and it looked really good in the dark fog. I am not sure how you could decide what vehicles should give off light or not - but anything to brighen the game helps.

@AaronVanGeffen
Copy link
Member

AaronVanGeffen commented Mar 19, 2020

What backend are you all using to test the lighting effects?

On Linux, I get screen/drawing invalidation problems as soon as I enable the lighting effects with the 'software (hardware passthrough)' backend. Having just the day/night cycle on does not present these problems. (This happens on develop as well, fwiw.)

Edit: #10973

@AaronVanGeffen AaronVanGeffen added changelog This issue/PR deserves a changelog entry. and removed pending rebase PR needs to be rebased. labels Mar 19, 2020
#ifdef __ENABLE_LIGHTFX__
if (lightfx_for_vehicles_is_available())
{
lightfx_add_lights_magic_vehicle((Vehicle*)spr);
Copy link
Member

Choose a reason for hiding this comment

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

Please use a named cast (static_cast, dynamic_cast, ...) instead of c-style cast. In this case looks like static would do

Copy link
Contributor Author

@WantDiscussion WantDiscussion Mar 19, 2020

Choose a reason for hiding this comment

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

static_cast<Vehicle*>(spr);

gives an "invalid type conversion" error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to

lightfx_add_lights_magic_vehicle(reinterpret_cast<Vehicle*>(const_cast<rct_sprite*>(spr)));

hope that's right

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine for now, thanks

src/openrct2/paint/sprite/Paint.Sprite.cpp Show resolved Hide resolved
@AaronVanGeffen
Copy link
Member

An idea for a future PR: add light sources to the shops/stalls, which are currently unlit.

@ocalhoun6
Copy link
Contributor

Hm, I'm not sure why the vehicles should give off light. That's not typically what happens at night - vehicles stay dark.

Some certainly would, like miniature railroad trains, streamlined monorail trains, and lift cabins. Heck, for that matter, a lot of flat rides might be lit up like christmas trees:
photo-1523113501336-6ea4a7d6660d

An idea for a future PR: add light sources to the shops/stalls, which are currently unlit.

Come to think of it, wouldn't most rides have their entire station platform lit up at night? For safety reasons if nothing else.

@Gymnasiast
Copy link
Member

@ocalhoun6 On funfairs, yes. In amusement parks: not so much.

Fix lights being off center in some rotations
Copy link
Member

@AaronVanGeffen AaronVanGeffen left a comment

Choose a reason for hiding this comment

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

I've noticed most of your commit messages just state which files have been edited. In the future, please stick to our commit message guidelines.

For now, I think we can merge this as one rephrased (squashed) commit.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

It might need a little bit of tweaking, but since it is optional and labelled experimental, and two other team members are ok with it, I'm ok too.

@AaronVanGeffen AaronVanGeffen merged commit 828eef7 into OpenRCT2:develop Mar 20, 2020
daandeheij pushed a commit to daandeheij/OpenRCT2 that referenced this pull request Mar 24, 2020
janisozaur added a commit that referenced this pull request Mar 24, 2020
- Feature: [#3154] Use a random title sequence each time it is shown.
- Feature: [#6553] Android version now runs in full screen.
- Feature: [#7865] Transport rides can now be synchronised.
- Feature: [#9073] Shortcut keys for the Tile Inspector.
- Feature: [#10305] Add two shortcuts for increasing and decreasing the scaling factor.
- Feature: [#10189] Make Track Designs work in multiplayer.
- Feature: [#10357] Added window for scenery scatter tool, allowing for area and density selection.
- Feature: [#10637] Console command to remove all floating objects.
- Change: [#1164] Use available translations for shortcut key bindings.
- Change: [#10997] Speed is automatically reset to normal upon scenario completion.
- Fix: [#2485] Hide Vertical Faces not applied to the edges of water.
- Fix: [#5249] No collision detection when building ride entrance at heights > 85.5m.
- Fix: [#6766] Changelog window doesn't open on some platforms.
- Fix: [#7784] Vehicle tab takes 1st car colour instead of tab_vehicle's colour.
- Fix: [#7854] Cannot build a custom spiral roller coaster design.
- Fix: [#7854] Empty entries in spiral roller coaster designs list.
- Fix: [#8151] Game freezes upon demolishing mazes at odd heights.
- Fix: [#8875] RCT1 competition scenarios are classified incorrectly.
- Fix: [#10176] Mistake in the sprite for the land tool's 6x6 grid.
- Fix: [#10196] Doors unable to be placed at end of track corners.
- Fix: [#10228] Can't import RCT1 Deluxe from Steam.
- Fix: [#10313] Path furniture can be placed on level crossings.
- Fix: [#10325] Crash when banners have no text.
- Fix: [#10376] No ratings generated when a shop and track intersect.
- Fix: [#10420] Money effect causing false positive desync.
- Fix: [#10477] Large Scenery cannot be placed higher using SHIFT.
- Fix: [#10489] Hosts last player action not being synchronized.
- Fix: [#10543] Secondary shop item prices are not imported correctly from RCT1 saves.
- Fix: [#10547] RCT1 parks have too many rides available.
- Fix: [#10587] Update last action coordinates on correct player.
- Fix: [#10631] Game bugs out and crashes if you get too many stations via copying stations with the tile inspector.
- Fix: [#10662] Duck cheat tooltips look odd and do not explain anything.
- Fix: [#10694] The lift hill speed of the flying roller coaster cannot be changed (original bug).
- Fix: [#10705] Apply multithreaded rendering to all viewports.
- Fix: [#10739] Mountain tool overlay for even-numbered selections.
- Fix: [#10752] Mute button state not correctly set at startup.
- Fix: [#10822] Can place too many peep spawns.
- Fix: [#10898] Banner text has an offset in tile inspector window.
- Fix: [#10904] RCT1/LL-scenarios with red water won't open.
- Fix: [#10941] The Clear Scenery tool gives refunds for ghost elements.
- Fix: [#10963] Light effects are drawn off-centre in some rotations.
- Fix: [#10993] Bottom toolbar not refreshing when a guest leaves the park.
- Fix: [#11001] Rides list does not use natural sorting.
- Fix: [objects#54] Stage Coach cars are not considered covered by the game.
- Fix: [objects#56] Handymen cut grass incorrectly.
- Improved: [#682] The staff patrol area is now drawn on the water, instead of on the surface under water.
- Improved: [#10858] Added horizontal grid lines to finance charts.
- Improved: [#10884] Added y-axes and labels to park window charts.
- Improved: [#10970] Introduced optional light effects for vehicles at night.
- Removed: [#6898] LOADMM and LOADRCT1 title sequence commands (use LOADSC instead).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This issue/PR deserves a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants