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.

Sign up for GitHub

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

Jump to bottom

allow disable shadow map auto update, fix shadow system init #3214

Merged
merged 1 commit into from
Nov 2, 2017
Merged

allow disable shadow map auto update, fix shadow system init #3214

merged 1 commit into from
Nov 2, 2017

Conversation

ngokevin
Copy link
Member

@ngokevin ngokevin commented Nov 2, 2017

Description:

Changes proposed:

  • Shadow system setup was never getting called.
  • Allow disable auto update shadow map to allow one-time shadow baking and on-demand updates.

donmccurdy
donmccurdy reviewed Nov 2, 2017
View reviewed changes
docs/components/shadow.md Outdated

| Property | Description | Default Value |
|--------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------------------------------|
| autoUpdate | Whether to dynamically update the shadow map every frame. Disable and manually update by setting `renderer.shadowMap.needsUpdate = true` for best performance. Calculating shadow maps are expensive. | true |
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/are expensive/is expensive

@dmarcos
Copy link
Member

dmarcos commented Nov 2, 2017

Tests failing

@ngokevin
Copy link
Member Author

ngokevin commented Nov 2, 2017

Updated.

needs test fixed label? GitHub shows red X everywhere already.

@dmarcos
Copy link
Member

dmarcos commented Nov 2, 2017 •
edited
Loading

It helps me visually and sometimes you get an X on the pull request list when in reality is not broken tests but some sort of code coverage compliance thing. It makes more explicit what it's pending for the people not familiar with travis

@ngokevin
Copy link
Member Author

ngokevin commented Nov 2, 2017 •
edited
Loading

OK I don't like more process, but it's fine. FYI those code coverage compliance things were legit from when people accidentally included .only in their tests (46c7cfc). I've written a hook to prevent that.

@dmarcos dmarcos merged commit 48faf8d into aframevr:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donmccurdy donmccurdy donmccurdy left review comments

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ngokevin @dmarcos @donmccurdy