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

feat: add quickstart models and sorting #155

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

TheLLspectre
Copy link
Contributor

Adding quickstart tab and sorting models.
Adding documentation.
Fix some variable naming.

Adding quickstart tab and sorting models.
Adding documentation.
Fix some variable naming.
@@ -12,12 +12,15 @@ public class Models : EditorWindow

#region Public Fields

public static List<ModelData> modelsQuickStart = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some comments to your newly added Lists

public static List<ModelData> modelsPrivate = new();
public static List<ModelData> modelsPublic = new();

public static List<TexturePair> texturesQuickStart = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some comments to your newly added Lists

public static List<TexturePair> texturesPrivate = new();
public static List<TexturePair> texturesPublic = new();

public static string privacyQuickStart = "quickstart";
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some comments to your newly added Lists

{
if (!isProcessing)
{
while (true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a while(true) here ? seems weird / useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So without this while, is fetching only one page of model.
But I'm agree, and may be we can use thoses lines to:

 PaginationTokenPublic = "";
 Debug.Log("no next page to fetch.");

To stop the process. I'll check this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix it !

With this fix we also fix another bug, which it was, sometines on redraw or reopening, first models or the first page were in double.

package/Editor/Models/ModelsUI.cs Outdated Show resolved Hide resolved
Adding documentations and also only call reloading tabs instead of all the window. This may fix the memory overhead ?
Fix fetching models processing with a better way to stop it.
Also remove setting the tab when opening window and only let it inside the OnEnable. Avoid double fetching.

With this fix we also fix another bug, which it was, sometines on redraw or reopening first models or the first page was in double.
@qvaleroo
Copy link
Collaborator

The model screen is not really usable :/
When I switch tabs, they disappear. If I go on page 2 of a tab and then change the tab, and go back, the bottom button is not in sync with what is displayed.
Really long to load, and the display is sporadic.
Can't we do better 🙏 ?
https://github.com/scenario-labs/Scenario-Unity/assets/95273550/1f0dd49c-504d-4ae9-a71d-2051a35abc6b

@Morgan-6Freedom
Copy link
Contributor

@qvaleroo can you validate that the last push fix your last comment ?

@qvaleroo
Copy link
Collaborator

qvaleroo commented Mar 15, 2024

The last fix forces the quick start to be opened first, but the tab still disappears when I select another one, and the display of models is still chaotic. But it is already the case in production.
This should be addressed really soon I think, but we can merge this one to address the Quickstart tab

@TheLLspectre
Copy link
Contributor Author

The last fix forces the quick start to be opened first, but the tab still disappears, and the display of models is still chaotic. But it is already the case in production. This should be addressed really soon I think, but we can merge this one to address the Quickstart tab

I think there is a subject about memory and SSL connection trouble.

@Morgan-6Freedom
Copy link
Contributor

@qvaleroo I merge this one, can you open a bug for the disappearance ?

@Morgan-6Freedom Morgan-6Freedom merged commit b7a9ec0 into develop Mar 15, 2024
1 check passed
@Morgan-6Freedom Morgan-6Freedom deleted the dev-quickstart-models branch March 15, 2024 15:08
@qvaleroo
Copy link
Collaborator

#157

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.

3 participants