Conversation
|
Dependencies should be added to the Workspace and Editor project. |
| { | ||
| public class RenderSpriteSystem : BaseSystem | ||
| { | ||
| public this(Application app) : base(app) {} |
There was a problem hiding this comment.
Same here with consistency.
There was a problem hiding this comment.
Ah, I was messing around with that and forgot to remove them since Application is a singleton now. If it's okay I will replace the existing App property and put Application.Instance instead.
There was a problem hiding this comment.
I think that is fine! No reason to allow more than one Application I think. As long as we don't think this will end potential support for more than one Window.
Engine/src/Window/Window.bf
Outdated
| /*Glfw.WindowHint(GlfwWindow.Hint.ContextVersionMajor, 4); | ||
| Glfw.WindowHint(GlfwWindow.Hint.ContextVersionMinor, 6); | ||
| Glfw.WindowHint(GlfwWindow.Hint.OpenGlProfile, .CoreProfile); | ||
| Glfw.WindowHint(GlfwWindow.Hint.OpenGlForwardCompat, Glfw.TRUE); | ||
| Glfw.WindowHint(GlfwWindow.Hint.OpenGlForwardCompat, Glfw.TRUE);*/ |
There was a problem hiding this comment.
This should have a comment explaining why we aren't using these hints for now.
There was a problem hiding this comment.
Yeah, it was some problems with ImGui, but I think Qzole fixed that. I will check
There was a problem hiding this comment.
It wasn’t fixed at the moment of reviewing this, but it might be with the new library. I will look into it later today. Thanks for pointing this out
Merge new changes from typo branch
Fusioon
left a comment
Fusioon
left a comment
There was a problem hiding this comment.
Pretty good overall. I did run into issue when creating new project which was caused by the Samples folder being in wrong path and some memory leaks/crashes all of which should have comment besides them.
Editor/src/Util.bf
Outdated
|
|
||
| var filePath = new String(); | ||
| var fileName = new String(); | ||
| var newFilePath = new String(); |
There was a problem hiding this comment.
Why aren't scope allocations used?
There was a problem hiding this comment.
Because if you have a deeper folder structure, it would allocate lots of strings on the stack, which would make it easier for a stack overflow. There would be 6 strings of unknown length for each level.
|
I will look into the other JSON library so that may fix the style serialization issue |
| { | ||
| var typeName = scope String(); | ||
| type.GetShortName(typeName); | ||
| if (typeName == "BaseComponent" || typeName == "BehaviourComponent") |
There was a problem hiding this comment.
The type name is BehaviorComponent (US spelling), not BehaviourComponent (UK spelling).
Is there a way to use reflection to get a nameof like in C#? https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/nameof
There was a problem hiding this comment.
Btw, because of this misspelling, an error shows up in the console when trying to add a BehaviorComponent
| var typeName = scope String(); | ||
| type.GetShortName(typeName); | ||
| if (typeName == "BaseComponent" || typeName == "BehaviourComponent") | ||
| continue; | ||
|
|
||
| if (ImGui.MenuItem(typeName)) | ||
| { | ||
| componentType = type; | ||
| _showAddComponentPopup = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Changing this to
var typeId = type.GetTypeId();
if (typeId == typeof(BaseComponent).GetTypeId() || typeId == typeof(BehaviorComponent).GetTypeId())
continue;
var typeName = scope String();
type.GetShortName(typeName);
if (ImGui.MenuItem(typeName))
{
componentType = type;
_showAddComponentPopup = false;
break;
}will make it harder for these mistakes to happen.
| { | ||
| var typeName = scope String(); | ||
| componentType.GetName(typeName); | ||
| Log.Error("Failed create component ({})", typeName); |
There was a problem hiding this comment.
Change "Failed create component ({})" to "Failed to create component ({})"
3 participants
Once the editor is merged with the master branch, some of the features in the renderer branch @Fusioon has, can be integrated into the editor for images and rendering and stuff.