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

Editing! Past canvas entries! And some others! Oh, my! #1149

Merged
merged 93 commits into from Apr 16, 2022

Conversation

Hans5958
Copy link
Member

@Hans5958 Hans5958 commented Apr 9, 2022

EDIT 3:

We moved! Visit #1314 (issue, recommended), #1315 (PR), and the remaster branch for the current status!


EDIT 2:

Despite of the name of the branch is "edit," this PR has expanded and actually tackles 5 issues at once.

  1. Editing (Feature request: Edit existing entries from the GUI #708)
    Editing existing entries from the website.
  2. Past/alternative canvas entries
    Adding/Editing new entries on past canvas/canvas variations.
  3. Multi-canvas paths
    Have multiple paths for different canvas on any period or variation
  4. New links format
    A links object that has both existing website and subreddit, and new types such as Discord links and wiki entries, along with separation by array (JSON) or newlines (website)
  5. Canvas variations
    Support for canvas variations other than the default r/place canvas. (e.g. The Final Clean)

EDIT: Now with past canvas entries, check the comments!


Should've resolves #708, but this is somehow closed. I'll just put it here.

This allows editing from the website, with adjustments on the script. This also include changes of the "submitted-by" string into "contributors" array. Additional script is made for merging temp-atlas.json with atlas.json, also made to check if there are edit conflicts, which will be excluded from addition.

The merge script is tested, but the crawler doesn't. So, please check it.

firefox_2022-04-09_21-44-53

@netlify
Copy link

netlify bot commented Apr 9, 2022

Deploy Preview for place-atlas ready!

Name Link
🔨 Latest commit 2980b89
🔍 Latest deploy log https://app.netlify.com/sites/place-atlas/deploys/625ace6b1b90870008aee431
😎 Deploy Preview https://deploy-preview-1149--place-atlas.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Hans5958 Hans5958 changed the title Editing! In the website! Oh, my! Editing! Periods! Oh, my! Apr 10, 2022
@Hans5958
Copy link
Member Author

Hans5958 commented Apr 10, 2022

Got entries for past canvas (aka time travel) working. The code checks the period value on the entry. The osu one has this following period.

{
	# ..., 
	"period": "0-14"
}

The gamemodes have the value of 4-14.

{
	# ..., 
	"period": "4-14"
}

Add that with editing...

I think it is good if the later TFC have the ID of 15 later.

@Hans5958 Hans5958 changed the title Editing! Periods! Oh, my! Editing! Past canvas entries! Oh, my! Apr 10, 2022
@nico-abram
Copy link
Member

Maybe we should use a bigger number for the image interval, like 0-100-...-1400 to leave some room in case we ever add more images?

web/_js/draw.js Outdated
if (startPeriodField.value === endPeriodField.value) {
exportObject.period = [startPeriodField.value]
} else if (startPeriodField.value * 1 < endPeriodField.value * 1) {
exportObject.period = [startPeriodField.value + "-" + endPeriodField.value]
Copy link

Choose a reason for hiding this comment

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

If I don't misread the code, this means the JSON contains the period number (0-n), right? It would be better to translate this to the timestamp using timeConfig for storage instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently I use the availability on the images. Using timestamp would be too fine for later atlas edits.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 10, 2022

Maybe we should use a bigger number for the image interval, like 0-100-...-1400 to leave some room in case we ever add more images?

That's why I think that this should be merged after the new canvases is added. Even so I think we can transpile the values if we are going to add new images in the future.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 12, 2022

ICYMI: The submitted_by is replaced by contributors array. This enables listing of multiple contributors on a single entry. Added that with the new period field, the current format is as follows. Even so, the `submitted_by" field will stay working.

{
	"id": "...",
	"contributors": [ "redditor1", "redditor2" ],
	"name": "...",
	"description": "...",
	"website": "...",
	"subreddit": "/r/subreddit1, /r/subreddit2",
	"center": [ 726.5, 727.5 ],
	"path": [
		[ 727.5, 679.5 ],
		[ 708.5, 681.5 ],
		// ... 
	],
	"period": "0-15"
}

I have an idea to actually merge all links into an object, and/or adding a Discord link and the wiki entry (inside of it). I also plan to convert all of it as an array for futureproofing. Something like this.

{
	"id": "...",
	"name": "...",
	"description": "...",
	"contributors": [ "redditor1", "redditor2" ],
	"links": {
		"website": [ "...", "..." ],
		"subreddit": [ "subreddit1", "subreddit2" ],
		"discord": [ "invitecode1", "invitecode2" ],
		"wiki": [ "Page 1", "Page 2" ]
	},
	"center": [ 726.5, 727.5 ],
	"path": [
		[ 727.5, 679.5 ],
		[ 708.5, 681.5 ],
		// ...
	],
	"period": "0-15"
}

@mxdanger
Copy link
Member

Have you considered art that has a changing path over time? Such as the expansion or recession of certain artwork during the time range.

@mxdanger
Copy link
Member

Also, I think it would be a good idea to turn subreddit into an array to be consistent with everything else in the links array.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 12, 2022

Have you considered art that has a changing path over time? Such as the expansion or recession of certain artwork during the time range.

I will copy the contents later with later edit, but please see https://discord.com/channels/960791635342524496/960814065901502546/962660023664795728.

Also, I think it would be a good idea to turn subreddit into an array to be consistent with everything else in the links array.

I mean I did?

@mxdanger
Copy link
Member

I mean I did?

If it's not a mistake, you have the two subreddits as one string with a comma instead of being two strings.

@Hans5958
Copy link
Member Author

Oh, I see. I meant two strings of course. Nice catch.

@mxdanger
Copy link
Member

mxdanger commented Apr 13, 2022

Here is a potential schema that allows for multi-path entries for past history. To keep it simple I think its best that its only incremental by the hour, so you only have a range from 0-82.

Note:

  • If the range value only includes the number, then that is the starting number, and it is inferred that 82 is the end number. This should only be used on the last range in the array.
    • 0, 67 would be invalid. 0-66, 67 would be valid.
  • None of the numbers in the range should overlap.
    • 0-62, 62-78 would be invalid. 0-62, 63-78 would be valid.

In this example the artwork exists from the start to hour 62. A replacement for the proposed period value.

{
    "id": "...",
    "name": "...",
    "description": "...",
    "contributors": [ "redditor1", "redditor2" ],
    "links": {
        "website": [ "...", "..." ],
        "subreddit": [ "subreddit1", "subreddit2" ],
        "discord": [ "invitecode1", "invitecode2" ],
        "wiki": [ "Page 1", "Page 2" ]
    },
    "paths": [
        "0-62", {
            "center": [ 726.5, 727.5 ],
            "path": [
                [ 727.5, 679.5 ],
                [ 708.5, 681.5 ]
            ]
        }
    ]
}

If the artwork was destroyed and then re-added then the following can be used. However, if the artwork significantly changes and moves location across the canvas, a new entry should be used instead.

{
    "paths": [
        "0-62", {
            "center": [ 726.5, 727.5 ],
            "path": [
                [ 727.5, 679.5 ],
                [ 708.5, 681.5 ]
            ]
        },
        "79", {
            "center": [ 726.5, 727.5 ],
            "path": [
                [ 727.5, 679.5 ],
                [ 708.5, 681.5 ]
            ]
        }
    ]
}

If the artwork existed from the beginning to the end then 0 can be used.

{
    "paths": [
        "0", {
            "center": [ 726.5, 727.5 ],
            "path": [
                [ 727.5, 679.5 ],
                [ 708.5, 681.5 ]
            ]
        }
    ]
}

Let me know your thoughts on if this is a good way to format multiple paths.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 13, 2022

I think "63-78", {} is not required. The current code only detects the given ranges so "4-6, 8-10" would mean it will appear on periods 4, 5, 6, 8, 9, and 10. "0" would mean only the period 0, not all periods, but we can do an "all" period.

Also, I agree with the 1 hour per image, but we'll see how the others think about this.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 13, 2022

ICYMI: Here is my analysis about how fine the timeline should be: https://gist.github.com/Hans5958/d9d6474c56abfde8c9000f4baf951575

@mxdanger
Copy link
Member

mxdanger commented Apr 13, 2022

I think "63-78", {} is not required.

I agree.

"0" would mean only the period 0, not all periods, but we can do an "all" period.

As I mentioned in my explanation: if the range value only includes the number, then that is the starting number, and it is inferred that 82 is the end number. If artwork really did exist for the first hour, then "0-1" would have to be used to be valid.

I think we should just keep it as numbers and not use "all".

@Hans5958
Copy link
Member Author

Oops, didn't catch the sentences right there. But that means that for one period, you would do something like 10-10, then? I would disagree on that one.

I think that is not consistent enough. IMO we should choose either using start points (e.g. 0, 20, 60, etc.), similar to the one you guys had previously on Discord, or using ranges (e.g. current implementation on period on the current state of the PR)

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 13, 2022

Since the final version The Final Clean is now out, gonna mention again my preference.

I think it is good if the later TFC have the ID of 15 [or] later the last, (with the default is the last hour).

(old footage)

@Codixer
Copy link
Member

Codixer commented Apr 13, 2022

If possible, do NOT make this part of the timeline. But a toggle somewhere. I want to keep the history of r/place and TFC seperate from each other.

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 13, 2022

If possible, do NOT make this part of the timeline. But a toggle somewhere. I want to keep the history of r/place and TFC seperate from each other.

Aww, fine then. So far I got two alternatives.

  1. A switch that switches between the real timeline slider and the alternate version slider. (It would just include TFC currently but maybe other alternate versions, maybe also the past drafts IDK)
  2. Just a dropdown that switches between the real timeline and the alternate versions. If it chooses other than the real timeline option, hide the slider.

@fabi321
Copy link
Contributor

fabi321 commented Apr 13, 2022

Maybe we could add periods like "0-" wich would be from start to end, or "-3, 6-10, 15-" wich would include everything from start to 3, 6, 7, 8, 9, 10 and everyting from 15 to end

@Hans5958
Copy link
Member Author

Hans5958 commented Apr 13, 2022

This might getting complicated on parsing the periods and not even thinking how to implement it on the edit tool...

@fabi321
Copy link
Contributor

fabi321 commented Apr 13, 2022

The parsing logic wouldn't bee too complicated. I think this sould translate to a list with multiple entries in the editor. But I do understand your concerns regarding how the draw/edit interface should be shaped to enable these powerful features while not being confusing.

@mxdanger
Copy link
Member

If we don't use a range number and instead only starting points then how should we signify the end of the range? Just a number with blank content? Such as: "67", {}

@mxdanger
Copy link
Member

mxdanger commented Apr 13, 2022

To revise my original schema, we could implicitly define the ending value so that just one dumber means it only happened during that time. So 10 would be valid and 0-82 would have to be used for start to finish.

I still think using a range would be easier to do for the UI as the user for example the user may have two rows with a range slider in the first column and a edit button next to it so the range 0-50 is displayed in the first row and 51-82 on the next row. If the user changes the range of the first one to 0-49 then a new row is created between the two that has a Plus button to suggest creating a new path with the range of 50.

web/_js/time.js Outdated Show resolved Hide resolved
web/_js/draw.js Outdated Show resolved Hide resolved
@Hans5958 Hans5958 force-pushed the edit branch 2 times, most recently from 304dd6b to 863e033 Compare April 16, 2022 13:50
@Hans5958 Hans5958 changed the base branch from cleanup to remaster April 16, 2022 15:21
@Hans5958 Hans5958 marked this pull request as ready for review April 16, 2022 15:21
@Codixer Codixer merged commit bc01405 into placeAtlas:remaster Apr 16, 2022
This was referenced Apr 16, 2022
@Hans5958
Copy link
Member Author

We will continue on #1314.

@Hans5958 Hans5958 added this to the Remaster milestone Apr 24, 2022
@Hans5958 Hans5958 deleted the edit branch April 7, 2023 12:29
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