-
Notifications
You must be signed in to change notification settings - Fork 6
Define custom shapes #158
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
Define custom shapes #158
Conversation
|
Wow nice! I'll take a look today. |
Smibu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments in the code, but at the same time I'd like to suggest a hopefully simpler implementation:
- Instead of storing the shapes as JSON files, could they be just normal lev files? This way they would be easily editable, and we wouldn't need custom serialization logic for them.
- Instead of storing a cached image of the shape, could we implement a
LevelControlthat inherits fromGLControlthat is able to directly render a given lev? So a CustomShapeControl would have a reference to a LevelControl instead of a PictureBox. I imagine this might not be a huge task because all the pieces are there already; we just need to glue them together. We could share the main window GL context. This maybe requires updating GLControl to the official version because it seems the current (vendored in repo) version might not support GL context sharing.
Elmanager/Geometry/GeometryUtils.cs
Outdated
| internal static (Vector center, Vector min, Vector max) CalculateBoundingBox(List<Polygon> polygons, List<LevObject> objects, | ||
| List<GraphicElement> graphicElements) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this method is not needed: if you construct a Level, it already has a method for computing its bounds.
|
|
||
| var (center, _, _) = GeometryUtils.CalculateBoundingBox(clonedPolygons, clonedObjects, clonedGraphicElements); | ||
|
|
||
| // Normalize positions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this normalization required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just felt it would be easier, and more clean that shapes are centered around origin than arbitrary positions.
Also, you need to do this step during translation and rotation, so doing it once before saving, instead of each time the shape is in use reduces the amount of calculations you need to do, unless I'm mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sense, but I think the normalization should probably be done when loading the shapes, rather than on save. Especially now that the shapes are editable as levs, we cannot assume the coordinates are normalized in the shape (lev) file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I'll look into changing it to on loading instead.
|
|
||
| protected GraphicElementDto(GraphicElement element) | ||
| { | ||
| Type = element.GetType().Name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid reflection. It's not trim-compatible, and my hope is that some day elmanager exe size could be greatly reduced via trimming as soon as WinForms supports trimming.
|
|
||
| public override void Write(Utf8JsonWriter writer, GraphicElementDto value, JsonSerializerOptions options) | ||
| { | ||
| JsonSerializer.Serialize(writer, value, value.GetType(), options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we should avoid reflection.
|
Thank you looking at my changes, and thanks for the great feedback! :) Using the LEV format makes sense, especially if they can be rendered directly in the selection dialog, but I have no experience with OpenGL which is why I didn't go down that road. I think it would make sense to do this in two steps.
|
Co-authored-by: Mika Lehtinen <Smibu@users.noreply.github.com>
I haven't removed the support for deserialization of JSON shapes to help with converting between the old JSON and the new LEV format.
|
I did a quick change to serialize to and from LEV instead of JSON. Using LEV format works well. I noticed that LEV objects require a start object (I got a random crash during conversion from JSON to LEV). It isn't a problem adding a start object as it is easy to filter them on load, but am I missing any other requirements for a Level to be "valid"? Here is an updated example gallery should you want to test it. |
|
I think just the start object and 1 (ground) polygon is required for validity. By two steps, do you mean two PRs? I would like just this one PR where we do both steps. You can regardless send some intermediate version for people to test in discord if you want to gather early feedback. |
Then the easiest solution is to limit custom shape creation to when the user has selected at least 1 polygon.
No, I just mean refactoring-wise. Refactor first to use LEV, and get rid of all of the unnecessary JSON serialization related code, and only then try to implement the Level rendering. Since I have 0 experience with GL, I don't know if I will be able to implement the last part :) |
|
Ah ye ok, that sounds good 👍 If the GL thing turns out difficult, I can take a look at it sometime. The new control might be useful in other contexts also. |
I haven’t started looking at it yet, but I have a question regarding how you see the LevelControl used. If you have a grid like the one I used in the shape selection dialog. Would the grid just consist of individual LevelControls, where each LevelControl has its own shared GL context? How would performance be in such a case? |
|
The grid would consist of CustomShapeControls (as it already is now?) and a CustomShapeControl would have a reference to a LevelControl (instead of a PictureBox). So you'd pass the LevelEditorForm's GLControl to ShapeGalleryForm so that it can be further passed to each CustomShapeControl and LevelControl as the shared context. I wouldn't expect performance problems; GL rendering should be pretty fast and hopefully the GLControl's paint method is only invoked when the control is actually visible (and not e.g. outside of a scroll area etc). |
|
I've played around with making a minimal example of a custom GLControl, and trying to just draw a triangle in a form and I can't even get anything drawn on screen. I tried changing to OpenTK.GLControl throughout, as it has SharedContext defined, and the Editor itself draws as usual. But I just get InvalidOperation errors in the test form/dialog. Really at a loss for what to do here .. any pointers? :D |
|
I need to see some code, hard to say otherwise. Maybe push some wip branch to your fork? |
See a very sloppy test implementation at: The dialog is loaded when you trigger Create Custom Shape, but you can move it elsewhere if necessary. I just wanted to create a minimal working example, where I'm using the shared context to do a basic drawing. I had to install a different package "OpenTK.GLControl" to get the SharedContext property in GLControl. |
|
Thanks :) it works with a few changes: d71f140 It was just missing the compat profile: Profile = ContextProfile.Compatability;(Works also with the timer thing, but probably OnPaint is enough, as shapes are just static drawings)
Ye, I mentioned you'll need to use the official newest version |
|
Great! I’ll try to get level rendering to work when I have time later this week. The timer thingy was just me being desperate to get something calling as I had problems with the OnPaint method initially. |
… to adjust if necessary
Smibu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, it's getting closer to finish line :) I added a few more comments here.
Found a bug (imo): if I place a shape, reopen form and cancel, it forgets the last selected shape. I think this used to work so maybe broke in refactoring.
|
|
||
| if (!IsHandleCreated) | ||
| { | ||
| SharedContext = sharedContext; // Set shared context before initialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this answers the question:
simply calling a control's constructor does not create the Handle.
So let's remove the if :)
Yeah I know, it happened after refactoring to storing the state in the _shapeSelection object. I left it for now but I can see if I can fix it. |
ExtraRendering shouldn't be needed because we have transient elements already and we don't have to render anything extra Co-authored-by: Mika Lehtinen <Smibu@users.noreply.github.com>
Co-authored-by: Mika Lehtinen <Smibu@users.noreply.github.com>
The handle should never be able to be created at this point anyways
This reverts commit a5146ac.
…election after closing Shape Selection From with the Cancel button
The bug has been fixed here: 3fe32b0 |
…rong LGR when opening Shape Selection Form
Smibu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This seems almost ready for merging. Can you update PR description to reflect current state? I will squash merge this, so PR title and description will be used as the commit message.
Co-authored-by: Mika Lehtinen <Smibu@users.noreply.github.com>
I've updated the pull request description (the first message in this pull request). Let me know if I should change anything :) |
|
Big thanks and congrats for getting this over the finish line. :) I will create a build later today and announce in discord. |
|
Thank you for all your help! Very much appreciated the time you put in to helping me get this finished :) |

This pull request adds a new Custom Shape Tool, called "Shape", to the Level Editor that allows you to quickly place predefined custom shapes into the current level.
Furthermore, this pull request also adds a new right click context menu option called "Save as shape..." which creates a new custom shape based on the current selection.
Custom shapes are saved as regular Elma levels (*.lev) on disk, and are grouped by sub-folders in the root folder for custom shapes. The root folder for custom shapes is called
sle_shapesand will be created in the same directory as Elmanager.exe the first time a custom shape is saved.The custom shape collection is just a folder structure with files, and therefore easily shareable / importable / exportable.
The following components can be added to custom shapes:
The new Custom Shape Tool supports the following actions:
Images
New Shape button, and Shape Selection dialog
Shapes are grouped by folders.
Example shapes for quick testing
Here is an example collection of shapes you can use for trying it out.
Place the following
sle_shapesfolder in the same folder as the Elmanager.exesle_shapes.zip