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

Porting material testers project to Godot 4 #616

Closed
wants to merge 1 commit into from
Closed

Porting material testers project to Godot 4 #616

wants to merge 1 commit into from

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented May 10, 2021 •
edited
Loading

Converting this to Godot 4 because we need this for testing the new renderer.

image

Note, currently two godot balls with broken materials that cause issues, see #50186 for a fix.

This was referenced May 10, 2021
Closed
Closed
@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch 2 times, most recently from 8f9bf96 to 9b7f8ba Compare May 10, 2021 05:51
@aaronfranke aaronfranke changed the base branch from master to 4.0-dev May 10, 2021 09:44
@aaronfranke aaronfranke added this to the After 4.0 milestone May 10, 2021
@aaronfranke
Copy link
Member

aaronfranke commented Jun 10, 2021 •
edited
Loading

@BastiaanOlij Soon we will begin porting the demos to the current Godot master branch in preparation for Godot 4 alpha (and therefore this PR wouldn't violate the first rule anymore 😉). Can you update this PR so that it works with the current master branch of Godot, and check if there is anything that needs doing before merging? The CI checks have failed.

@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch 2 times, most recently from 4a5a274 to 73c3a51 Compare June 14, 2021 04:17
@BastiaanOlij
Copy link
Contributor Author

@BastiaanOlij Soon we will begin porting the demos to the current Godot master branch in preparation for Godot 4 alpha (and therefore this PR wouldn't violate the first rule anymore 😉). Can you update this PR so that it works with the current master branch of Godot, and check if there is anything that needs doing before merging? The CI checks have failed.

I've brought it up to todays master, but it still fails when I unhide the meshes with a material that use SSR.

@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch from 73c3a51 to 75636af Compare June 14, 2021 04:20
aaronfranke
aaronfranke requested changes Jun 16, 2021
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The improvements that are applicable to Godot 3.x (adding FPS display and a quit button) can be in a separate PR if you want, and then this would be rebased on top of that.

It's fine if the demo doesn't 100% work on master yet, we can improve it further later.

3d/material_testers/tester.gd Outdated Show resolved Hide resolved
3d/material_testers/tester.gd Outdated Show resolved Hide resolved
3d/material_testers/tester.gd Outdated Show resolved Hide resolved
@BastiaanOlij BastiaanOlij mentioned this pull request Jul 3, 2021
Closed
@BastiaanOlij
Copy link
Contributor Author

This PR fixes the Godot Balls that were hidden: godotengine/godot#50186
I'll unhide them on the assumption that this PR will be merged today.

@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch from 75636af to 85ec539 Compare July 6, 2021 00:47
@BastiaanOlij
Copy link
Contributor Author

@aaronfranke you're changes should be in, I did keep the static types for nodes.

@BastiaanOlij
Copy link
Contributor Author

I've added two reflection probes around the mirror ball to test reflections. There are still issues to be solved here

@BastiaanOlij BastiaanOlij mentioned this pull request Jul 6, 2021
Closed
@BastiaanOlij
Copy link
Contributor Author

Note that godotengine/godot#50196 adds logic that allows a single reflection probe to do a decent job in this use case.

@akien-mga akien-mga mentioned this pull request Jul 26, 2021
Merged
@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch from 85ec539 to 2fa1180 Compare July 26, 2021 10:20
@aaronfranke
Copy link
Member

@BastiaanOlij Can you rebase this PR and ensure it works on the latest master branch of Godot?

@BastiaanOlij
Copy link
Contributor Author

Owh dang, the import files have changed for the new godot 3 version, thats gonne be fun :) I'll have a look

@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch 3 times, most recently from b22b5c6 to c52fe29 Compare March 28, 2022 09:58
@BastiaanOlij BastiaanOlij force-pushed the material_testers_4.0 branch from c52fe29 to cfba591 Compare March 28, 2022 10:08
@BastiaanOlij BastiaanOlij marked this pull request as ready for review March 28, 2022 10:08
@BastiaanOlij
Copy link
Contributor Author

Ok, that was a bit of a nightmarish rebase, it looks like everything works again but hopefully nothing went missing.

aaronfranke
aaronfranke reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I accidentally forgot this PR existed and ported the Material Testers demo myself. However, it's likely that there are things I missed that are included in this PR, so we'll still try to merge this.

3d/material_testers/backgrounds/experiment.hdr.import
path.s3tc="res://.godot/imported/experiment.hdr-6856dc9c7216ce389b450cda78cd0dd4.s3tc.ctex"
path.etc2="res://.godot/imported/experiment.hdr-6856dc9c7216ce389b450cda78cd0dd4.etc2.ctex"
uid="uid://bc5g4j4hpwts4"
path="res://.godot/imported/experiment.hdr-6856dc9c7216ce389b450cda78cd0dd4.ctex"
metadata={
"imported_formats": ["s3tc", "etc2"],
Copy link
Member

@aaronfranke aaronfranke Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't look right. If there are two imported formats, why is there only one imported path? When I dragged these into the sky to use as a sky texture, it automatically set two imported paths.

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 don't know to be honest, I'm not sure what controls this

@Calinou Calinou mentioned this pull request Jan 9, 2023
Merged
@akien-mga akien-mga changed the base branch from 4.0-dev to master February 28, 2023 17:41
@Anutrix
Copy link

Anutrix commented Apr 16, 2023

After #823, what parts of this are remaining or needed?

@Calinou
Copy link
Member

Calinou commented Apr 16, 2023

#823 fully supersedes this PR, closing. Thanks for the contribution nonetheless!

@Calinou Calinou closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfranke aaronfranke aaronfranke requested changes

Assignees
No one assigned
Labels
archived update
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@BastiaanOlij @aaronfranke @Anutrix @Calinou