search

accassid / dice-gen

A three.js based dice generator react app.
17 stars 7 forks source link

The gleep/add save project support #89

Closed Earl-Brown closed 3 years ago

Earl-Brown commented 3 years ago

I was a little frustrated with always having to select the font, so I decided to add project management features.

Changes made:

There is an issue with how the d4 is handled which causes the d4 to not render correctly after loading. I don't know enough with this code to actually fix that issue, so I would like help with that.

When switching to the D4 after loading the save file, this sequence of errors is repeated 24 times:

index.js:1 THREE.TextGeometry: font parameter is not an instance of THREE.Font. 
    at D4FaceGeometry (http://localhost:3000/static/js/main.chunk.js:4965:3)
    at mesh
    at Face (http://localhost:3000/static/js/main.chunk.js:5178:3)
    at Die (http://localhost:3000/static/js/main.chunk.js:4567:87)
    at Canvas (http://localhost:3000/static/js/0.chunk.js:361865:60)
index.js:1 THREE.Geometry.fromBufferGeometry(): Position attribute required for conversion. 
    at D4FaceGeometry (http://localhost:3000/static/js/main.chunk.js:4965:3)
    at mesh
    at Face (http://localhost:3000/static/js/main.chunk.js:5178:3)
    at Die (http://localhost:3000/static/js/main.chunk.js:4567:87)
    at Canvas (http://localhost:3000/static/js/0.chunk.js:361865:60)

I have tried calling setGlobalFont() after loading the project, but switching to the d4 still doesn't render the faces.

This is an example of a saved config file, created by

  1. refreshing the browser
  2. selecting a font
  3. saving the file

dicegen-config.zip

Earl-Brown commented 3 years ago

One other thing to note - I'm not proud of the output preparation code, but I'm waiting to get the solution to the current bug before I clean it up.

accassid commented 3 years ago

Alright, so there's a lot here.

  1. The D4 issue is actually not an issue with the D4, it's that all the other dice are overlooking a type issue. They all use CombinedTextGeometry which is a custom Geometry I made that combines glyphs more effectively than the default. That Geometry does not throw an exception like the original. You can resolve your current issue by replacing the use of TextGeometry in D4FaceGeometry with CombinedTextGeometry. Although, that will require some extra work to make the subtraction correct. I'll create a separate issue for that.
  2. AntDesign has a fileloader built in, I don't think you need browser-fs-access. Take a look at how fonts are a being loaded in for reference. Any time we can avoid installing an extra library is a win.
  3. I wouldn't get fancy with our own file extension. As long as we're using json, no reason not to use the json extension.
  4. I'll have to think more about the concept of dumping and loading the state. It's certainly the most simple and elegant solution. However, I've hesitated to do this myself for a while for several reasons. Mainly, as soon as we add this feature, users will always assume that their files, no matter the version can be loaded in. What that means is if anyone makes any change to even just the name of a state prop, it will either no longer load older versions of that property, or they will need to add a specific exception for older versions of the file.
Earl-Brown commented 3 years ago

Thank you so much for the feedback!1 - I'll change d4faces, but I really doubt I'll be the one who fixes subtraction!2 - I concur with the new library, but didn't know about existing options.  Does the antd have file save support as well?  Would it be better to DIY a file saver?3 - just use .json.  Sure...folks can change it if they want.4 - versioning.  Yeah.I didn't take changed variable names into account in what I designed, but I did do a little bit of a test to make sure that adding variables wouldn't break anything.  How to make that easier for developers?  Hrmm.  Maybe add an easy way to define changes, and the reducer would look for it?  Probably need to be something in the base types, but I have no idea how that would work.If you can't tell, I'm really a TypeScript noob.  LOOOOTS of JavaScript experience, but I'm learning TypeScript from what I'm doing here (not why I started making changes, but it's a nice bonus).

-------- Original Message -------- Subject: Re: [accassid/dice-gen] The gleep/add save project support (#89) From: Andrew Cassidy notifications@github.com Date: Wed, February 24, 2021 3:01 pm To: accassid/dice-gen dice-gen@noreply.github.com Cc: Earl github@theGleep.com, Author author@noreply.github.com

Alright, so there's a lot here. The D4 issue is actually not an issue with the D4, it's that all the other dice are overlooking a type issue. They all use CombinedTextGeometry which is a custom Geometry I made that combines glyphs more effectively than the default. That Geometry does not throw an exception like the original. You can resolve your current issue by replacing the use of TextGeometry in D4FaceGeometry with CombinedTextGeometry. Although, that will require some extra work to make the subtraction correct. I'll create a separate issue for that. AntDesign has a fileloader built in, I don't think you need browser-fs-access. Take a look at how fonts are a being loaded in for reference. Any time we can avoid installing an extra library is a win. I wouldn't get fancy with our own file extension. As long as we're using json, no reason not to use the json extension. I'll have to think more about the concept of dumping and loading the state. It's certainly the most simple and elegant solution. However, I've hesitated to do this myself for a while for several reasons. Mainly, as soon as we add this feature, users will always assume that their files, no matter the version can be loaded in. What that means is if anyone makes any change to even just the name of a state prop, it will either no longer load older versions of that property, or they will need to add a specific exception for older versions of the file. —You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or unsubscribe.

accassid commented 3 years ago

There's a vanilla JS file download function I made for downloading the STL files in src/utils/downloader.ts. If you wanted to reuse that, you could add another boolean parameter to the function to download without zipping.

Earl-Brown commented 3 years ago
  1. AntDesign has a fileloader built in, I don't think you need browser-fs-access. Take a look at how fonts are a being loaded in for reference. Any time we can avoid installing an extra library is a win.

I've been thinking about the file save experience, and one thing that's really stuck out to me is there is no file naming option. Using browser-fs-access gives us that option, as well as selecting where we will save the project file to.

If I were to remove it, I would at least want to add a "What do you want to name your project" popup.

Earl-Brown commented 3 years ago

I've made a number of changes, but there is one "any" warning that I just can't figure out how to get rid of - would LOVE some feedback on that!

Also, I changed TextGeometry to CombinedTextGeometry as suggested, but I get the same errors when I try to save the file with the D4 selected.

Earl-Brown commented 3 years ago

I'm going to close this version and start again from the latest code. Hopefully that will yield better code!