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

Native glowing windows support #2153

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
6 participants
@akortunov
Copy link
Contributor

akortunov commented Jan 30, 2019

Feature #4836.

Adds an internal ESP-less glowing system for both game and editor. It does not spawn any objects and does not alter any records, just affects rendering, so it is a purely graphics feature.

An approach for game itself is a quite simple:

  1. Calculate current daytime state once per frame during weather update using settings imported from Morrowind.ini.
  2. Add a callback for every osg::Switch with given name. This callback is supposed to read data from weather manager and select active child node.

For edtor we just update windows state via visitor when we change lighting mode - for exterior cells windows will glow in the "night" mode.

This feature does not affect performance with default assets since they have no NiSwitchNodes.

For testing you can use meshes from Glow in the Dahrk. On my hardware, glowing windows decrease FPS a bit because of NiUVController's for flickering, but not because if this feature itself.

Since this feature is often used, can we add a fast and reliable implementaion to the engine?

Also I am not sure which callbacks are more suitable - Update or Cull ones.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jan 30, 2019

This feature does not affect performance with default assets since they have no NiSwitchNodes.

Great!

For testing you can use meshes from Glow in the Dahrk. On my hardware, glowing windows decrease FPS a bit because of NiUVController's for flickering, but not because if this feature itself.

Is this solvable? What's do you mean by 'a bit' ;)

Since this feature is often used, can we add a fast and reliable implementaion to the engine?

If this would solve the above issue, then I believe that it falls under this PR.

Also I am not sure which callbacks are more suitable - Update or Cull ones.

@AnyOldName3 any advice here?

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Jan 30, 2019

Is this solvable? What's do you mean by 'a bit' ;)

1-2 FPS. It is because of flickering animation, so an only way to avoid it is to remove NiUVControllers from meshes at all. But a classic Windows Glow with NiFlipController's has a way worse performance (5-8 FPS loss for me in large cities).

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jan 30, 2019

I'm concerned about the code being typecasted to "windows" where I could see where it could be generalized to just Glowing effect which can be used for any number of things not having to do with windows. :)

This is a totally different effect than other 'glowing' effects a sword for example or is that a particle effect? Or magick?

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Jan 30, 2019

Technically, this feature is about day-night mode swtch. Meshes may not have a glow at all.
I suppose a more generic names should be used, but I have no idea which ones.

Which switch indeces are used:
0 - a default mesh, used during day in exteriors or at night (or when the weather is not bright) in interiors.
1 - a night mode for exteriors.
2 - a daytime mode for interiors (may contain sunrays, for example, which should not be present in exteriors).

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Jan 30, 2019

And what's up with AppVeyor builds?

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jan 30, 2019

Perhaps a naming convention keeping in line with DayNightCycle?

Not just AV but also RTD, github is dropping it's 'services' features. We'll have to step over to using webhooks, if that is possible.

https://docs.readthedocs.io/en/latest/webhooks.html#webhook-github-services
https://developer.github.com/changes/2018-04-25-github-services-deprecation/

AV still works fine on gitlab (because they do it correctly).

So... perhaps... maybe... you guys should start using gitlab and MRs instead of github please? :)

@terabyte25

This comment has been minimized.

Copy link
Contributor

terabyte25 commented Jan 30, 2019

Mmm, looking back at GitLab, MRs are much better than PRs, at least for me now.

@akortunov akortunov force-pushed the akortunov:glowing_windows branch from 0c0caf0 to 81f7393 Jan 30, 2019

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Jan 30, 2019

I renamed variables to the NightDayMode ones since this feature is not limited to glowing windows.
In theory, you can implement any object which should look differently depending on time of day.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jan 30, 2019

I see isExterior being used... does this mean that day/night cycle for internal areas too? For example... we're inside, would the windows be light up like it was sunny outside or dark if at night? (inverse of being outside at night, glowing windows)

That's the point of NightDayMode right? ( just clarifying )

@wareya

This comment has been minimized.

Copy link
Contributor

wareya commented Jan 31, 2019

does this mean that day/night cycle for internal areas too?

2 - a daytime mode for interiors (may contain sunrays, for example, which should not be present in exteriors).

@psi29a psi29a requested a review from AnyOldName3 Jan 31, 2019

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jan 31, 2019

Last call for review? @Capostrophic @AnyOldName3

@AnyOldName3

This comment has been minimized.

Copy link
Member

AnyOldName3 commented Jan 31, 2019

Also I am not sure which callbacks are more suitable - Update or Cull ones.

@AnyOldName3 any advice here?

I think you were right to go with an update callback for this.

@akortunov akortunov force-pushed the akortunov:glowing_windows branch from 81f7393 to 47af96d Jan 31, 2019

@akortunov akortunov force-pushed the akortunov:glowing_windows branch from 47af96d to 9e4a339 Feb 2, 2019

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Feb 2, 2019

Any other objections?

@Capostrophic

This comment has been minimized.

Copy link
Contributor

Capostrophic commented Feb 2, 2019

I'll try actually testing this in-game first.

@Capostrophic

This comment has been minimized.

Copy link
Contributor

Capostrophic commented Feb 2, 2019

Nothing obvious comes up at the first glance.

@psi29a psi29a changed the title [Discussion] Native glowing windows support Native glowing windows support Feb 4, 2019

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Feb 4, 2019

Merging, thanks.

@psi29a psi29a merged commit f4313c0 into OpenMW:master Feb 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.