Skip to content

Conversation

remi-chapelain
Copy link
Contributor

@remi-chapelain remi-chapelain commented Oct 18, 2021

Purpose of this PR

Following this (now closed) PR, it has been decided that it was better to include this in the material sample to avoid making the HDRP package bigger by sharing some of the materials / textures.

133756088-929300eb-f339-4035-9a1c-eed99660d66a

Moreover, given we want to reduce the size of the Editor, the size of the material sample have been drastically reduced.

  • Initial material sample size: 69.3mb
  • Merged transparency + material: 72.95mb
  • Optimized new material sample: 36.27mb

What has been done mostly is converting the TGA to TIFF and reducing the size of unnecessary larger texture (2048 and 1024px).

53e5bf93ce22c3838080f6b7f6ca4437

It's still possible to go lower (still some large textures) but at the cost of losing some quality when getting close to the materials.


Testing status

  • Imported the sample in a fresh new HDRP project ✔️
  • Opened all the scenes independently ✔️

Comments to reviewers

Added @Vic-Cooper to have feedback on documentation and text that is inside the scene. Also I would like to know if you think it's interesting to include some of those bit in the documentation itself for redundancy.
For simplicity purpose, here's a gdoc with all the text.

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions github-actions bot added the HDRP label Oct 18, 2021
@remi-chapelain remi-chapelain changed the title adding transparency scenes to material sample [HDRP] Adding transparency setup scenes to material sample Oct 18, 2021
@remi-chapelain remi-chapelain requested review from a team, AlixMi and sebastienlagarde October 19, 2021 08:50
@remi-chapelain remi-chapelain marked this pull request as ready for review October 19, 2021 08:51
@Vic-Cooper
Copy link
Contributor

I asked my lead @IritArkinUnity about this, and in-context text doesn't fall under our docs remit. As such, you can publish this PR without my approval. If there's new information in this scene that isn't in the docs, please create a ticket for this task and we can scope the docs additions into a future sprint.

Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

I reviewed the samples and listed the issues in the doc

@remi-chapelain remi-chapelain requested a review from iM0ve November 15, 2021 11:51
@remi-chapelain
Copy link
Contributor Author

Fixed most of the issue on the feedback document. 👍
There's still some concerns we can't really fix because of limitations by how the samples are delivered (i.e bad HDRP asset setup etc..)

Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

My found issues were addressed. Maybe still worth having a pass on the wording/text

@remi-chapelain
Copy link
Contributor Author

Had a last discussion with @iM0ve and re-organized the scenes to highlights more the setup differences between them.

b8fca283f153d37c1a4e2cc92706811f.mp4

Ready to merge ✔️

@sebastienlagarde sebastienlagarde merged commit cf4f9dc into master Nov 22, 2021
@sebastienlagarde sebastienlagarde deleted the hd/add-transparency-scenes-to-material-sample branch November 22, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants