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

add Env to NME Preview #14398

Merged
merged 21 commits into from
Oct 10, 2023
Merged

add Env to NME Preview #14398

merged 21 commits into from
Oct 10, 2023

Conversation

onekit-boss
Copy link
Contributor

We can custom Env, Image and toggle Env / Background color now.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 6, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 6, 2023

@RaananW RaananW requested a review from deltakosh October 6, 2023 15:48
Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

Just to be sure we are ok with the GUI, here's how it appears:
image

cc @PatrickRyanMS

@sebavan
Copy link
Member

sebavan commented Oct 9, 2023

LGTM if @PatrickRyanMS is all good with it :-)

@sebavan
Copy link
Member

sebavan commented Oct 9, 2023

Also one last thought for @RaananW to be sure importing .env would not break any kindz of buildzzzz 🐝🐝🐝🐝🐝

@RaananW
Copy link
Member

RaananW commented Oct 10, 2023

Also one last thought for @RaananW to be sure importing .env would not break any kindz of buildzzzz 🐝🐝🐝🐝🐝

Works fine, as this is just a static file in the public directory.
Might cause an issue when used as a package, as you will need to deliver the file yourself.

@onekit-boss
Copy link
Contributor Author

Also one last thought for @RaananW to be sure importing .env would not break any kindz of buildzzzz 🐝🐝🐝🐝🐝

Works fine, as this is just a static file in the public directory. Might cause an issue when used as a package, as you will need to deliver the file yourself.

okay, I has remove it, and use "https://assets.babylonjs.com/environments/environmentSpecular.env"

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Nice thanks a lot !!! a last change and I can merge it in :-) Could you make "https://assets.babylonjs.com/environments/environmentSpecular.env" a public static so that users can point it at their own URL if need be :-)

@onekit-boss
Copy link
Contributor Author

Nice thanks a lot !!! a last change and I can merge it in :-) Could you make "https://assets.babylonjs.com/environments/environmentSpecular.env" a public static so that users can point it at their own URL if need be :-)

yes. Now it is a exists URL of babylon assets (https://assets.babylonjs.com), not a file .

@onekit-boss
Copy link
Contributor Author

Nice thanks a lot !!! a last change and I can merge it in :-) Could you make "https://assets.babylonjs.com/environments/environmentSpecular.env" a public static so that users can point it at their own URL if need be :-)

yes. Now it is a exists URL of babylon assets (https://assets.babylonjs.com), not a file .

I use this URL as other source codes of babylon.js .your can find the URL in bellow files:
packages\dev\core\src\Helpers\environmentHelper.ts
packages\tools\sandbox\src\tools\environmentTools.ts

@sebavan
Copy link
Member

sebavan commented Oct 10, 2023

yes but in environmentHelper users can override it here https://github.com/BabylonJS/Babylon.js/blob/master/packages/dev/core/src/Helpers/environmentHelper.ts#L344 and sandbox is not meant to be used as a framework.

Users needs to be able to control where they download assets from.

@onekit-boss
Copy link
Contributor Author

yes but in environmentHelper users can override it here https://github.com/BabylonJS/Babylon.js/blob/master/packages/dev/core/src/Helpers/environmentHelper.ts#L344 and sandbox is not meant to be used as a framework.

Users needs to be able to control where they download assets from.

your mean is. ........"user need custom a env of the NME Preview window?" . I has add a "upload button" on the right of "model ComboBox" in NME Preview window. so user can override the default env "https://assets.babylonjs.com/environments/environmentSpecular.env".

@sebavan
Copy link
Member

sebavan commented Oct 10, 2023

Oh no, sorry, I just mean if you move it in a static public field or a let in the module so a developer consuming the npm package can change it before loading nme.

@onekit-boss
Copy link
Contributor Author

a public static

or...... your mean is a public static filed of the class?

@sebavan
Copy link
Member

sebavan commented Oct 10, 2023

something like this:

public static DefaultEnvironmentURL = "https://assets.babylonjs.com/environments/environmentSpecular.env"';

private _refreshPreviewMesh(force?: boolean) {
        switch (this._globalState.envType) {
            case PreviewType.Room:
                this._hdrTexture = new CubeTexture(DefaultEnvironmentURL , this._scene);
                if (this._hdrTexture) {
                    this._prepareBackgroundHDR();
                }
                break;
            case PreviewType.Custom: {
                const blob = new Blob([this._globalState.envFile], { type: "octet/stream" });
                const reader = new FileReader();
                reader.onload = (evt) => {
                    const dataurl = evt.target!.result as string;
                    this._hdrTexture = new CubeTexture(dataurl, this._scene, undefined, false, undefined, undefined, undefined, undefined, undefined, ".env");
                    this._prepareBackgroundHDR();
                };
                reader.readAsDataURL(blob);
                break;
            }
        }

@onekit-boss
Copy link
Contributor Author

something like this:

public static DefaultEnvironmentURL = "https://assets.babylonjs.com/environments/environmentSpecular.env"';

private _refreshPreviewMesh(force?: boolean) {
        switch (this._globalState.envType) {
            case PreviewType.Room:
                this._hdrTexture = new CubeTexture(DefaultEnvironmentURL , this._scene);
                if (this._hdrTexture) {
                    this._prepareBackgroundHDR();
                }
                break;
            case PreviewType.Custom: {
                const blob = new Blob([this._globalState.envFile], { type: "octet/stream" });
                const reader = new FileReader();
                reader.onload = (evt) => {
                    const dataurl = evt.target!.result as string;
                    this._hdrTexture = new CubeTexture(dataurl, this._scene, undefined, false, undefined, undefined, undefined, undefined, undefined, ".env");
                    this._prepareBackgroundHDR();
                };
                reader.readAsDataURL(blob);
                break;
            }
        }

thanks. I got it :)

@sebavan sebavan merged commit 9883543 into BabylonJS:master Oct 10, 2023
10 checks passed
@PatrickRyanMS
Copy link
Member

Just to be sure we are ok with the GUI, here's how it appears: image

cc @PatrickRyanMS

sorry for the late response, I have been out of the office. The only thing I see is that the icon for environment is a little small in comparison to the rest on the bar. It should be sized to fit with the size of the other icons. If needed, I can provide a new icon, but it would be preferable to use the same svg if possible. Something like this:

image

@sebavan
Copy link
Member

sebavan commented Oct 10, 2023

I ll adapt this one ASAP

@sebavan
Copy link
Member

sebavan commented Oct 10, 2023

@PatrickRyanMS it looks real strange zoomed in like it would be a bold font :-( could you create another svg ? like the other ones in the bar ?

@PatrickRyanMS
Copy link
Member

I can make a new svg to fit with the current icons. Give me a few minutes.

@PatrickRyanMS
Copy link
Member

@sebavan, here's the updated icon. This is saved at the same size as the rest of the icons, so the css should be the same:
environment_24_regular.zip

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants